Add Accordion Block #64119
Add Accordion Block #64119
Conversation
|
Size Change: +6.04 kB (+0.34%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
jffng
added
[Type] Enhancement
jffng
force-pushed
the
try/accordion-block
branch
2 times, most recently
from
c22c920 to
f6726e8
Compare
| $p->set_attribute( 'data-wp-class--is-open', 'state.isOpen' ); | ||
| if ( $attributes['openByDefault'] ) { | ||
| $p->set_attribute( 'data-wp-init', 'callbacks.open' ); | ||
| } |
There was a problem hiding this comment.
As you are using JavaScript to open the item, the initial HTML sent to the browser will have the item closed and will only open after the JavaScript loads, causing a layout shift.
It would be better if the state on the server could already contain the correct information about which item is opened so that the HTML the browser receives is the correct HTML.
There are several ways to share information between blocks. One option could be something similar to what we've done in the image block, where we put the info in the global state and only an id in the local context. But it's not the only way, so feel free to choose the best one for you.
(By the way, I am preparing a new guide on server-side rendering. Once I open the pull request, I will put a link here.)
There was a problem hiding this comment.
It would be better if the state on the server could already contain the correct information about which item is opened so that the HTML the browser receives is the correct HTML.
I tried to initialize the state on the server: 4c27428
There are two issues now:
- I'm still getting layout shift, I don't know why this is the case since I thought the state is being initialized with the right info on the server.
- It breaks if there is more than one accordion group on the page and
autoclose(only allows one item per group to be open at a time) is enabled, since they are sharing the same store and I'm using global state to track which ids are open.
Do you have any suggestions? Thank you for the review so far.
There was a problem hiding this comment.
I'm still getting layout shift, I don't know why this is the case since I thought the state is being initialized with the right info on the server.
The derived state was missing on the server.
It breaks if there is more than one accordion group on the page and autoclose (only allows one item per group to be open at a time) is enabled, since they are sharing the same store and I'm using global state to track which ids are open.
To fix the autoclose issue, I refactored the code to move the array with the IDs of open items to the local context.
I've made a video to explain the two changes.
There was a problem hiding this comment.
Thanks for the updates and video! Super helpful, and it's very useful to see how the local context is initialized on the server using a closure.
There was a problem hiding this comment.
Code-wise seems good to me. It'll probably be ready to go once you've reviewed how to share functions between different Gutenberg blocks 🙂
EDIT: I've labeled the pull request with "Needs Accessibility Review" to ensure this block also undergoes an accessibility review.
| } | ||
|
|
||
| $p = new WP_HTML_Tag_Processor( $content ); | ||
| $unique_id = wp_unique_id( 'accordion-item-' ); |
There was a problem hiding this comment.
Let's leave it with wp_unique_id for the moment 👍
jffng
force-pushed
the
try/accordion-block
branch
from
704e207 to
1b83689
Compare
|
I plan to follow up with some design and UX improvements, and will mark the PR ready for review after that. |
jffng
force-pushed
the
try/accordion-block
branch
from
8545dce to
15f408c
Compare
|
@jffng Picking up convo from over here.
I polled some other designers who regularly build sites, and the vote was to keep it an h3 (although both work IMO). I have a feeling h5 will most likely be too small in most themes.
Yeah, although this is a bit weird, it makes functional sense, and should be included in the wording. What about this for the Toggle: "Auto-close" |
jffng
requested review from
spacedmonkey,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
Flaky tests detected in fd87e7d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10357492397
|
This comment was marked as resolved.
This comment was marked as resolved.
|
Just noting that it would be good to make the Heading level attribute extensible as we recently did with other Core blocks that use the Here's the original PR for reference: #63535 |
I agree that h3 can be default because usually the sections that have an accordion have a h2. |
There was a problem hiding this comment.
This is looking amazing, thank you for working on it ✨
When testing it out, I noticed that the accordions are difficult to close in the Editor when clicking on the close button, but maybe this is on purpose, as you also need to be able to click to edit the title:
Screen.Recording.2024-08-14.at.12.20.04.mov
They close easily if I click anywhere outside the accordion content.
Otherwise, this is testing on the front-end and in the Editor really well for me.
I've also tested using a screen reader, and I believe all the notices that are read out sound correct. I am also able to tab through the accordions in the Editor to edit their titles and contents. However, it would be great to see more specific accessibility feedback before this lands. I did notice a small potential change to the way aria-labelledby and aria-controls are used, I think both may need their own unique id references.
|
I've addressed the outstanding feedback, and added the |
|
A quick high-level feedback from looking at Accordion Pattern as presented at ARIA Authoring Practices:
I would be in favor of naming the block accordingly instead of Trigger and Content as these names presented in the UI might be misleading. In particular, there is already Post Content block that is presented in the UI as Content, so there would be ambiguity.
In #63689, @creativecoder works on the Tabs block. Do you think the same rationale should drive the final implementation of that block that initially was coded using only two blocks Tabs and Tab, which would be similar to having Accordion Group and Accordion. By the way, the Details block, which behaves like a pair of accordion header and panel combined together, is a single block with inner blocks provided by the user. Concluding, it would be good to establish a similar decision making process for all core blocks so the final user experience is coherent for users. |
Renaming to Accordion Header and Accordion Panel makes sense to me.
I think it would be best if the editing experiences for the Tabs and Accordion blocks were aligned. After a lot of experimentation, I found that treating the Accordion Header and Accordion Panel as separate blocks was the best way to allow users to achieve most designs using the existing block style supports, while following the ARIA authoring practices for this pattern.
The details block is quite limited stylistically — for example you can't control the spacing of and between the summary and content, and text and background colors get applied to the entire block. Consolidating the Accordion Header and Panel to a single block with inner blocks would result in the same stylistic limitations.
I agree! |
|
I support using the multiple blocks, the details block also started out with multiple blocks and more style options, but was reduced on request by reviewers. Which is one of the reasons why we need a more flexible accordion block. |
Note that I anticipate needing to break out the tab block into tab-label and tab-panel. Because the style supports system is set up to add controls for the block as a whole, I think we'll need to separate these so they can have independent style controls (background, border, margin/padding, etc) |
- Label the None icon setting - Add help text to block settings - Remove button focus styles - Add border support to the group.
…a shared function between blocks.
jffng
force-pushed
the
try/accordion-block
branch
from
aebf612 to
db03ba8
Compare
What
Adds several experimental blocks to the block library to enable an Accordion pattern.
Video Demo
https://cldup.com/transcoded/Oy55bzki_B.mov.mp4
Why
See #21584 and #63501
How
Accordion Group>Accordion Item>Accordion Trigger>Accordion Content. This allows the user to style the trigger and content separately, while still maintaining the accessibility and semantic requirements of an accordion. I explored an approach with only two blocks — accordion group and item — but this made it difficult to provide the user with enough styling control over the trigger and content.Testing Instructions
Playground link to test the PR in browser.
Test block markup