Rename Button describedBy prop to description and deprecate old name. #63486
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
afercia
added
[Type] Code Quality
|
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: +365 B (+0.02%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
tyxla
reviewed
Jul 15, 2024
There was a problem hiding this comment.
This solution makes sense to me, thanks @afercia 👍
I'll let the rest of @WordPress/gutenberg-components review it as it involves some prop deprecation.
I was just curious about which pre-existing instances of describedBy we choose to address.
| @@ -64,7 +64,7 @@ function BlockAlignmentUI( { | |||
| } ), | |||
| } | |||
| : { | |||
| toggleProps: { describedBy: __( 'Change alignment' ) }, | |||
| toggleProps: { description: __( 'Change alignment' ) }, | |||
There was a problem hiding this comment.
I see we've not updated the prop to the new one everywhere. Example:
Is that intended?
There was a problem hiding this comment.
Glad to update more occurrences if desired. I think I only searched for occurrences of \tdescribedBy= and \tdescribedBy: 😆
There was a problem hiding this comment.
I think that will make sense since otherwise folks might get unexpected deprecation warnings in core.
afercia
force-pushed
the
update/button-describedby-prop-rename
branch
from
92cfa5c
to
45e8c15
Compare
|
Thanks @tyxla I updated that occurrence and rebased. |
ciampo
reviewed
Jul 16, 2024
| @@ -32,7 +32,7 @@ function AlignmentUI( { | |||
| onChange, | |||
| alignmentControls = DEFAULT_ALIGNMENT_CONTROLS, | |||
| label = __( 'Align text' ), | |||
| describedBy = __( 'Change text alignment' ), | |||
There was a problem hiding this comment.
This is probably a breaking change for this component. The easy way to avoid the braking change would be to keep the describedBy prop, and internally pass it to the description prop in toggleProps.
Alternatively, we'd probably need to do something similar to what we've done in Button, where we support the old version alongside the new version.
There was a problem hiding this comment.
@ciampo I understand the potential concern. However, I couldn't find any usage of the describedBy prop for the AlignmentControl component. The prop is not even documented in the AlignmentControl readme. This component is used internally in the block-editor, is this change a real concern?
Rather, I'd say it's a good opportunity to document it better. I discovered a few more issues and inconsistencies that I'll detail in a new comment.
There was a problem hiding this comment.
This component is used internally in the block-editor
This change would affect the APIs of AlignmentControl and AlignmentToolbar, which are both public APIs of the @wordpress/block-editor package.
However, given that:
- as you mentioned, the property is not documented;
- as you mentioned, we couldn't find any current usages of the prop;
- any user using the old
describedByprop won't experience breaking, but only an unexpected description (which fallback to the default "Change text alignment") - the change is an overall improvement and aligns with the changes in the
Buttoncomponent;
I agree with you that it's ok to just swap the prop name from describedBy to describe
|
The AlignmentControl component of the block editor uses the AlignmentUI component which accepts a few props, including I couldn't find any usage of the In the Table block, the alignment control passes a label prop with value
Screen readers will announce both the label and description, thus providing a very confusing information to usdrs. Screenshots:
Inconsistencies with similar componentsThere are two very similar components: BlockAlignmentUI BlockVerticalAlignmentUI I will try to split all the above in a new issue. Native filesI found one more occurrence in the |
Sounds good — you can probably open a new PR(s) directly with changes to better align label and description text.
cc @WordPress/native-mobile (I will also tag you in a code comment to pinpoint the exact change) |
ciampo
reviewed
Jul 17, 2024
| @@ -77,7 +77,7 @@ function BlockAlignmentUI( { | |||
| }; | |||
| } ), | |||
| popoverProps: POPOVER_PROPS, | |||
| toggleProps: { describedBy: __( 'Change alignment' ) }, | |||
| toggleProps: { description: __( 'Change alignment' ) }, | |||
2 tasks
|
Pending final review and approval 😄 |
ciampo
approved these changes
Jul 17, 2024
|
Thank you, @afercia 🚀 |
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
…WordPress#63486) * Rename Button describedBy prop to description and deprecate old name. * Update readme. * Remove leftover and add changelog entry. * Update one more occurrence of describedBy prop. * Update one more describedBy occurrence in BlockAlignmentUI native file. Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: geriux <geriux@git.wordpress.org>
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.
Fixes #63483
What?
The Button
describedByprop name is misleading.Why?
Prop names should make clear what they are about and what their value type is.
How?
describedBytodescriptionand deprecates old name.Testing Instructions
Change block type or style.Outdent list item.Testing Instructions for Keyboard
Screenshots or screencast