Details block: add name attribute for exclusive accordion #56971
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
Soean
added
[Type] Enhancement
Soean
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
Size Change: +124 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 ab2b680. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7182873955
|
fabiankaegy
reviewed
Dec 12, 2023
There was a problem hiding this comment.
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. |
t-hamano
force-pushed
the
details-accordion
branch
from
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!
|
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.
What?
Fixes #56969
See HTML Living standard of WHATWG: https://html.spec.whatwg.org/multipage/interactive-elements.html#the-details-element
How?
Adds a
nameattribute 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: