-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Block Inspector: Avoid advanced panel only settings tab #47474
Conversation
Size Change: -567 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
Flaky tests detected in 0426466. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4022751500
|
It turns out the Table block doesn't render any extra settings controls while in its placeholder state but does once the rows and columns have been selected. This makes whether tabs are rendered inconsistent for the same block and a likely accessibility issue. Would it be sufficient to explore rendering the extra table settings controls even in its placeholder state so the tabs always display? I'm not sure how else we might guard against this issue. Screen.Recording.2023-01-27.at.11.25.03.am.mp4 |
Same with the Image block.
What exactly is the issue? The addition of tabs, if settings are available? |
I think so in most cases. The Image block height/width isn't particularly useful yet, but when image aspect ratio/default values are applied to placedholders — then it would be. It's also worth noting this sets us up to explore having "Settings" as the default tab, if there are settings available. Instead of the current flow, where "Appearance" is open by default. |
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
I think that makes sense but it's probably worth getting an accessibility perspective on it. |
For the table block, I think it'd be ok to show the 'Fixed width' option as something the user can selected before having created a table, but the header and footer options don't make sense until the rest of the table size has been defined. Otherwise on the frontend users can end up with a table with a header or footer but no body. |
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.
Code looks good to me 👍
Thanks for the reviews and discussion, everyone 👍
That's it in a nutshell. That the same block type may have tabs or not in different circumstances. My understanding of previous a11y feedback on these tabs was that at least the same block type should have a consistent set of tabs i.e. if the Table block is going to have both styles and settings tabs, it should do so at all times.
Good point.
This is enough to always present the Settings tab for the Table block, which in turn keeps the tabs consistent between placeholder and non-placeholder states. I believe this is inline with previous a11y feedback. I'll get this merged and create follow-up PRs for the Table and Image blocks. |
A PR ensuring the settings tab for the Table block by rendering the "Fixed width table cells" control, even for the placeholder state, is up in #47536.
I've taken a quick look at this one. As noted, the image sizes aren't particularly useful until they affect the placeholder. Adding these controls to the settings tab for the Image block's placeholder state isn't as trivial as for the Table block. So it might take some time to settle on an approach, meaning we'll miss the cut-off for 15.1 and, therefore the WP 6.2 feature freeze. |
Fixes: #47463
What?
It avoids rendering a Settings tab in the majority of cases where it would only contain an advanced panel.
The only exception here is when a block already has multiple tabs (i.e. both the list view and style tabs) as the advanced panel doesn't really fit within the other available tabs or beneath the entire tab panel.
Why?
We don't need a tab for only the advanced panel. It is standard across almost all blocks.
How?
useInspectorControlsTabs
hooks logic to omit the check for advanced panel fills when determining which tabs should be rendered unless the block has the list view tab.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast