-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Button: Improve the label of the button block in list view #74163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| ...a, | ||
| text: ( a.text || '' ) + text, | ||
| } ), | ||
| __experimentalLabel( attributes ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably time to make this stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it's time. I think we even had an issue with it.
|
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: +70 B (0%) Total Size: 2.57 MB
ℹ️ View Unchanged
|
|
It seems fine, motivation wise. Just to be sure, is this label announced to screen readers only when the button is in icon-only mode? |
|
My motivation is the new list view being explored in #74120 (it shows "button", "button", "button" as children even when the buttons have some nice content, it doesn't feel ideal) |
jasmussen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tentative 👍 👍, and I'd encourage others to chime in as well.
SirLouen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youknowriad if you try these unit tests in #74175 you may notice that they fail, in case of attributes are null or undefined this might return an error.
For the rest, test a couple of extra cases that seem to be passing well.
|
Why would the function be called with |
|
Feel free to push the other tests here :) |
If gutenberg/packages/block-editor/src/components/block-title/use-block-display-title.js Lines 53 to 54 in a22ec1e
It will return PS: Changed base of #74175 |
talldan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thsi works well, thanks for addressing the feedback on the other PR.
I guess I leave it up to you whether you want to make it more defensive before merging. Looks like some of the other instances of __experimentalLabel also destructure attributes, and I've not seen any bug reports.
|
I'm not a big fan of defensive coding in general, I feel like it's better to address issues at the root. In this case, if the assumption of the The other thing is that I do agree however that there's an issue with whenever |
|
This is one place where moving to TS is actually the right path forward, it will help us ensure all the assumptions are actually codified. |
t-hamano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we do that? and not update it consistently in all contexts do you know? |
Makes sense indeed. |
I don't remember, but at least the label for the block toolbar button seem unnatural without taking the context into account:
More recently, I've been considering adding a new |
For consistency with other blocks, I'm not addressing the null/undefined case.
Mamaduka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8281e8e to
146ae80
Compare
t-hamano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !
|
Flaky tests detected in 146ae80. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20459299663
|



This PR updates the Button block to use a label based on its content.
The need for this is more visible in #74120
Testing instructions