-
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
Details block: add name attribute for exclusive accordion #56971
Conversation
Size Change: +135 B (+0.01%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
With this underlying feature it may be nice to in the future add an "Accordion" wrapper block that automatically handles the name generation & assignment for any descendant detail blocks intern so users don't have to manually tinker with the names where they can easily make typos etc. And in an ideal world all the details elements would then also sync there styling similar to how the query loop post template does it. But these are all just future improvement ideas. Love the addition :) Will review the code in minute 👍 |
Flaky tests detected in 0a4786d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12824222463
|
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.
I left two minor comments. It works as intended and is a great enhancement.
Though I'm a little worried about the browser support at this point in time.
Yes Safari TP shipped it yesterday but looking at the global usage we are at less than 1% https://caniuse.com/mdn-html_elements_details_name
I'm worried introducing the setting now will be confusing because for the large majority of users the setting will have no effect.
I share the same concern. I'm also not sure most folks would understand what "name" is, even with help text. Maybe developers, but that's about it. |
@richtabor that is why I think the wrapper accordion block would be nice. Because it could abstract that away |
Maybe it would be an option to move the setting to the advanced panel for now |
I don't think the 1% is correct yet, but I agree, we should wait a little longer before we merge it into the release. Maybe it will soon be supported by Firefox.
You are right, this should be an advanced feature. |
This comment was marked as outdated.
This comment was marked as outdated.
Browser support is now 78% .. 1% to 78% in 4 months |
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @thomas-price, @sntran, @dogee, @sfadschm. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
I think this is fine. Are there any objections to the actual functionality? I'm fine with keeping it under Settings too—"Advanced" should be as consistent as possible in my view, especially as we continue to add controls across blocks, like overrides in synced patterns. "Title Attribute" and "Red Attribute" are in the Navigation Link blocks' Settings panel for example. I think this copy explains it a bit better:
|
The only problem is that Firefox doesn't support this attribute yet. As far as I can tell, there doesn't seem to be any progress on this issue. |
I would like to add my opinion if that's okay. It's fine for this not working in Firefox. The field is available in the Inspector. If you know what |
It is supported by Firefox: https://developer.mozilla.org/en-US/blog/html-details-exclusive-accordions/ |
Yes, now that it is supported in all major browsers I think we can move forward with this PR. |
@Soean Is it possible to proceed with this PR again? |
Just bumping this. It would be wonderful to have. Looks like it's now supported across current browser versions. |
ab2b680
to
38c2acd
Compare
I've resolved the conflicts and made it work with the latest Gutenberg. Regarding the help text, I've adopted the suggestion in this comment. Any feedback is welcome! |
Thank you som much @t-hamano ! Can't say anything code-wise but I tested this PR on playground in Chrome and Firefox and it works like a charm. |
Update: I've resolved the conflict again. I've also changed it to reference the HTML element via the source field to save the name attribute value. @WordPress/gutenberg-core Now this PR is ready, and I'd be happy to receive any final feedback. Summary of this PR:
|
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.
I tested it on Windows 11 without issues:
Chrome Version 132.0.6834.84 (Official Build) (64-bit)
Firefox 134.0.1 (64-bit)
I don't mind that the label for the field is "Name attribute", even if it is technical, the help text explains what the purpose is.
I don't think the word "attribute" will discourage people from using it. It can also be updated again if there is feedback from more users.
If browser support is still a concern, maybe the help text could be updated to include something along the lines of "in supported browsers".
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.
Also in favor of merging this now :)
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.
Good work!
Do we want a dev note to showcase the feature?
@Mamaduka given that it isn't really a change but rather a feature addition I don't think a Dev Note is needed. We should however feature it in the "Whats new in Gutenberg" post when it is released and given the scope of 6.8 I could see this being included in the highlight grid etc. |
Thank you everyone for the reviews! |
What?
Fixes #56969
See HTML Living standard of WHATWG: https://html.spec.whatwg.org/multipage/interactive-elements.html#the-details-element
How?
Adds a
name
attribute to the details block.Demo
accordion-details.mov
Backend
Testing Instructions
Add multiple details blocks .
Supported browser: https://caniuse.com/mdn-html_elements_details_name
Chrome 120, Edge 120, Safari 17.2, Firefox is not supporting this attribute yet.
Example content: