Rich text: fix props on DOM element warnings #31883
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
ellatrix
commented
May 17, 2021
| 'fontStyle', | ||
| 'minWidth', | ||
| 'maxWidth', | ||
| 'setRef', |
There was a problem hiding this comment.
Could we use ref here instead?
There was a problem hiding this comment.
If we use ref we would get the reference of the RichText component, right? If that's the case, it won't work because here what we need is the reference of the underneath native view of Aztec (RCTAztecView).
There was a problem hiding this comment.
Would forwardRef not work?
There was a problem hiding this comment.
Good point, yeah, we could forward the ref upstream instead of using setRef 👍 .
ellatrix
commented
May 17, 2021
| 'fontWeight', | ||
| 'fontStyle', | ||
| 'minWidth', | ||
| 'maxWidth', |
There was a problem hiding this comment.
Is there a style equivalent for Native to avoid all these style props?
There was a problem hiding this comment.
I think in theory we could extract them from the style object that is passed to the native view 🤔 .
ellatrix
commented
May 17, 2021
| 'selectionColor', | ||
| 'tagsToEliminate', | ||
| 'rootTagsToEliminate', | ||
| 'disableEditingMenu', |
There was a problem hiding this comment.
What does this do? What is being disabled?
There was a problem hiding this comment.
It's used to control triggering the suggestions by keycodes, besides for iOS, this value also determines whether the user can edit the attributes of the text in the text field.
ellatrix
commented
May 17, 2021
| 'textAlign', | ||
| 'selectionColor', | ||
| 'tagsToEliminate', | ||
| 'rootTagsToEliminate', |
There was a problem hiding this comment.
These props are used to clean up any undesired tags produced by Aztec (the native renderer we use for rich text), here is the related code:
gutenberg/packages/rich-text/src/component/index.native.js
Lines 247 to 266 in c290539
ellatrix
commented
May 17, 2021
| 'deleteEnter', | ||
| 'placeholderTextColor', | ||
| 'textAlign', | ||
| 'selectionColor', |
There was a problem hiding this comment.
Should a block be able to set selection color?
There was a problem hiding this comment.
I think we do still want this though as it enables a bit of control over color accessibility. The problem is that the selection highlight box and cursor can blend into the block's background color when we only use the platform's Native color choice. For the case of buttons, we have it set to match the text color as an attempt to provide some contrast.
Here's the original issue that was opened for this to get some more context wordpress-mobile/gutenberg-mobile#1773
|
Size Change: +117 B (0%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
In most cases, these props are required by the native module that we use for rendering rich text (Aztec). I've gone by one by one and add a brief description of the purpose:
|
|
Let's merge this PR so we get rid of the warnings. Perhaps we should mark all these props unstable until we can find a common solution for both web and native. |
|
@ellatrix It should be safe to merge this PR as it only involves changes in the web version of |
|
@fluiddot The props are set by the block, but for web the props don't do anything and they are passed to the DOM element, for which React logs a warning. |
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.
Description
These props are only used for the native version on RichText and should be removed before passing props to the DOM element, otherwise warnings are logged.
We should also better align on these props. They were all added as stable props to RichText, yet we don't intend so support them for the web version.
@hypest Could we get some clarification as to why each of these props are needed?
How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).