-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Separator block: Allow divs to be used as separators #67530
Conversation
Curious about your thoughts on this @jasmussen |
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: +176 B (+0.01%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
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.
Things worked well for me, and the code looks good 👍
It seems advanced. I'm not against it, but it seems important that the default is useful to useful to screen readers. It seems like this default is likely to be unchanged by most theme authors unless they have fairly deep knowledge of the behavior. That's also an aegument to ensure this control lives in the "Advanced" section, like the tag selector for groups and the like. |
@youknowriad, you need to update this object to fix unit tests - it is just testing the block's edit component unit. gutenberg/packages/block-library/src/separator/test/edit.js Lines 21 to 26 in cc5a2b7
|
Looks like the native tests stopped working because we're using some old Mac image or something. |
It looks like. There's an old PR for update - #59070. |
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, thanks!
Co-authored-by: youknowriad <[email protected]> Co-authored-by: jorgefilipecosta <[email protected]> Co-authored-by: Mamaduka <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: luminuu <[email protected]>
Co-authored-by: youknowriad <[email protected]> Co-authored-by: jorgefilipecosta <[email protected]> Co-authored-by: Mamaduka <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: luminuu <[email protected]>
closes #54976
What?
In some situations, you might want the separator blocks to not be announced by screen readers. You want them to be just design tokens. This PRs allows that by adding a "tagName" attribute to the separator block. Also adds a help text to explain when you should be using "divs".
Testing Instructions
1- Insert the separator block.
2- Under "Advanced" in the inspector panel, you can switch to "div".
3- Check that the separator still looks and behaves properly (we might want to check different themes, including classic ones)