Preview: skip rendering rich text #60544
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +232 B (0%) Total Size: 1.73 MB
ℹ️ View Unchanged
|
ellatrix
force-pushed
the
try/preview-skip-rich-text
branch
from
e82dc1d to
4700624
Compare
youknowriad
reviewed
Apr 8, 2024
| disableLineBreaks, | ||
| __unstableAllowPrefixTransformations, | ||
| readOnly, | ||
| ...contentProps |
There was a problem hiding this comment.
I wonder if using "opt-in" (listing the content props and omitting the rest) is clearer than "opt-out" here. Also why remove "native props" from preview, aren't these "style related" previews that are important for previews?
There was a problem hiding this comment.
We can't do that because we don't know all the possible attributes people could be passing to the tag. I mean, we can't unless we list all possible React props.
There was a problem hiding this comment.
ok but what about this part "aren't these "style related" previews that are important for previews?"
in other words, why are we calling removeNativeProps here?
There was a problem hiding this comment.
Block authors can pass all sorts of attributes to rich text, it's not just styles. Classes, data attributes, and all other valid HTML attribute names. We are removing native props because these are component props, not attribute names, and otherwise React will throw warnings that these aren't valid attribute names when trying to set them.
There was a problem hiding this comment.
Ok I got confused by the "native" in the function name. I thought it was about removing regular DOM attributes rather than removing "native like mobile native" RichText version attribute.
youknowriad
approved these changes
Apr 8, 2024
|
I'm a bit unsure about the large swings in the navigate metric. Sometimes it's a fairly large negative, sometimes fairly large positive. Let's merge and see how that metric will be affected in the graph I guess. |
cbravobernal
pushed a commit
to garridinsi/gutenberg
that referenced
this pull request
Apr 9, 2024
Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Mamaduka
reviewed
Apr 30, 2024
| value, | ||
| tagName: Tag, | ||
| multiline, | ||
| format, |
There was a problem hiding this comment.
@ellatrix, is the format prop used anywhere?
There was a problem hiding this comment.
It's a very old prop. I believe years ago at the start of GB we had string and children. We don't check it anymore, instead we rely on the type of the value (string or array). But we also shouldn't remove this because we'd be spreading it on a plain tag. Maybe it's ok because it's just a React warning in dev mode?
There was a problem hiding this comment.
Got it; it seems fine to keep it around.
There are no warnings. I was looking into React Native code and noticed this prop, but I wasn't sure if it was used anywhere.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Rich text executes lots of things. Let's see if omitting this from previews makes any improvement.
Why?
templates: -7.67% first run, -6.67% second,
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast