a8cteam51 / special-projects-blocks-monorepo

MIT License
6 stars 0 forks source link

Implement Accordion Block Plugin #16

Open jffng opened 2 months ago

jffng commented 2 months ago

What This PR implements an Accordion block. It's an alternative to #6.

Screenshot 2024-07-19 at 4 03 59โ€ฏPM

This approach actually is composed using four blocks:

Without getting too much into the weeds, this was done because it was the simplest way I could figure out how to allow user styling of the trigger and content separately, while maintaining the accessibility and semantic needs of an accordion pattern.

Highlights

Demo & Download https://cool-tools.mystagingwebsite.com/2024/07/12/accordion/

Video Walkthrough https://video.wordpress.com/embed/AfzRE5Om

jffng commented 2 months ago

@jarekmorawski @Nyiriland would you be able to give this a review when you have a chance? ๐Ÿ™

jarekmorawski commented 1 month ago

Just took it for a spin. Great work, @jffng! I assure you it'll make a big difference for 1000s of Woo stores out there. It's much more capable and easier to use than core's Details block. I also like the animation. It brings the block on par with other existing solutions out there.

Without getting too much into the weeds, this was done because it was the simplest way I could figure out how to allow user styling of the trigger and controls separately, while maintaining the accessibility and semantic needs of an accordion pattern.

I wonder if we should call the block Accordion Group or Accordions, then? And Accordion Item to just Accordion? And add a small inline + in the bottom right corner that would let users easily add new items? It seems the block works similarly to the Buttons block so the more UX we can re-use, the better for the user.

Clicking the black + would immediately insert a new Accordion block.

image

Other thoughts:

image image image image

As for the inner blocks, I'd rename Accordion Trigger to Trigger and Accordion Container to just Content and change the icons to some that are visually related and share a common metaphor (the arrow). See icons in Figma

image

That's all I had. Again, great work. Thanks for doing this!

Nyiriland commented 1 month ago

Then under Styles, users could decide if they want to keep the icon or not (Default would mean the icon is on). When "Text only" is active, we wouldn't show the Icon section in Settings.

Instead of introducing style options, what if we add a "None" option for the icon? The benefit is that this keeps related options together, and if developers want to extend the block by adding new styles, they won't compete with a "no icon" option (since you can only add one block style via the current Styles UI).

image image

image

image

From testing, you can add the border to Accordion Item, but not padding. If you add padding to the bottom of the Accordion Content, it does this:

https://github.com/user-attachments/assets/e8c30c6b-2da1-4e8c-9885-9129e4c4a43c

jarekmorawski commented 1 month ago

Instead of introducing style options, what if we add a "None" option for the icon?

Yeah, that's another possibility. There's no consensus on how such binary styles (icon vs. no icon) should be handled on the block side so we have some freedom.

Should the accordion be closed, unless you select the block, or have "open by default" checked?

Agreed!

My concern is that it might feel over-engineered for amateur users compared to the details block, but maybe that's needed for a more comprehensive block system:

That's my concern too but I had a chat with Jeff and it seems there's no other solution that wouldn't break a11y and diverge from HTML semantics. In the original design, the structure was much simpler.

image

Perhaps it's worth a second look? I can ask Woo block engineers to share their thoughts.

From testing, you can add the border to Accordion Item, but not padding. If you add padding to the bottom of the Accordion Content, it does this:

It could be a bug. I don't think we should reveal the content when the accordion is not open.

@Nyiriland, I was able to recreate that layout using a combination of Accordion Trigger padding and block spacing added to both Accordion Item and Accordion (the parent block). It's a bit convoluted, but well, designing complex nested layouts in Figma isn't a walk in the park either. ๐Ÿ˜‰

image

Icons: are there default icons in the WordPress UI library that we can utilize instead of adding new ones?

We can, but they may confuse users who may be familiar with blocks that already use those icons. We use custom icons all the time at Woo and it doesn't seem to be an issue. If we were to go with the default icons, I'd suggest:

image
jffng commented 1 month ago

Thank you @jarekmorawski @Nyiriland, great feedback!

I wonder if we should call the block Accordion Group or Accordions, then? And Accordion Item to just Accordion?

The way I understand the UX pattern and its naming, is that a single Accordion contains several sections of content that can be expanded and collapsed. The visual metaphor is of an instrument that expands in one place and contracts in another. So technically, there should be multiple sections (or "items") to make it an accordion, otherwise there the distinction between an Accordion and Details would be even more blurry. Does that make sense? Sorry if it's a bit esoteric.

As for the inner blocks, I'd rename Accordion Trigger to Trigger and Accordion Container to just Content and change the icons to some that are visually related and share a common metaphor (the arrow).

๐Ÿ‘ I agree, sounds good.

Do the "accordion trigger" and "accordion content" need to be separate item in the block list view?

Perhaps it's worth a second look? I can ask Woo block engineers to share their thoughts.

I'm open to ideas. I hit a wall in this PR https://github.com/a8cteam51/special-projects-blocks-monorepo/pull/6 which tries to do that but fell short of the requirements, particularly the ability to easily style the trigger, separately from the content, using the existing block supports and maintaining semantics.

Something to keep in mind with the design of the block's editing experience:ย there needs to be a way to associate a trigger with its corresponding content. This is what the Item does โ€” it just associates a trigger with content and disallows either from being moved or removed, and any other block types from being inserted, since doing so would break the functionality and UX pattern.

This approach also allows the user to insert as many Items into the Accordion as needed. I couldn't think of another way to keep the trigger and content in the right order. (AFAIK there is not a simple way in the block editor API to say, "only allow inserting a content block after a trigger")

From testing, you can add the border to Accordion Item, but not padding. If you add padding to the bottom of the Accordion Content, it does this:

It could be a bug. I don't think we should reveal the content when the accordion is not open.

Yeah, that's a bug. It's related to using a CSS-only approach to animating the show / hide behavior. With this approach,ย no hacky JS height calculations are needed, and it's responsive, which is great. But the result is that bug happens when padding is added to the Content container block. To get around it, I would need to disallow setting top and bottom padding on the Content block. This way, to add vertical padding to the Content, the user would have to wrap the blocks inside Content in a group block. It's convoluted, but I think a worthwhile tradeoff. What do you think?

Icons: are there default icons in the WordPress UI library that we can utilize instead of adding new ones?

I think we should supply custom icons, the core ones should be left specific to their respective blocks and settings.

I'll get started on the rest of the feedback and let you know when it's ready for another test.

Nyiriland commented 1 month ago

We can, but they may confuse users who may be familiar with blocks that already use those icons.

Sorry, I was referring to the icons for the trigger, not the block itself. The Twentig plugin extends the Details block to create accordion options, and I was noticing that the trigger icons look a lot like the WP icon set. If they're using the same ones, we probably can, too. (We chose not to extend the Details block because the markup was not built in an accessible way.)

image

But if not, the icons @jarekmorawski used in his mockups suffice.

jffng commented 1 month ago

@jarekmorawski for some reason when I try to export the Content block icon from Figma and load it into the block, I'm getting this:

image

The other block icons look alright, do you know of a way that might export the icon correctly?

Nyiriland commented 1 month ago

Yeah, that's a bug. It's related to using a CSS-only approach to animating the show / hide behavior. With this approach, no hacky JS height calculations are needed, and it's responsive, which is great. But the result is that bug happens when padding is added to the Content container block.

I wouldn't even call it a bug, it's how padding works. ๐Ÿ˜† I'm more concerned this is how people might expect to apply padding/spacing to block content, so we should account for that.

To get around it, I would need to disallow setting top and bottom padding on the Content block. This way, to add vertical padding to the Content, the user would have to wrap the blocks inside Content in a group block. It's convoluted, but I think a worthwhile tradeoff. What do you think?

I agree that a CSS-only solution for expand/collapse is cool, but I don't love having to wrap content in a group block just to be able to add spacing around it; I don't think it's an intuitive solution for beginner users that just want to be able to throw in content and expect it to look "ok". In my opinion, that includes having enough spacing between the accordion content and the block below it.

Idea: is it possible to add a simple div around the Content that shows/hides it? That way, we can apply padding/spacing to the Content, but it wouldn't be visible in the closed state.

Nyiriland commented 1 month ago
image

Oh one more thing: for this option I'd default the block to allowing multiple items open. It's better for UX in my opinion, and Nielson Norman agrees:

This problem can further escalate when an accordion automatically closes when another is opened, restricting usersโ€™ ability to combine information from multiple accordions at the same time. If you expect users to need information from several accordions at once, it is better to display all the content at once (even if it results in a longer page).

jarekmorawski commented 1 month ago

So technically, there should be multiple sections (or "items")

In such a case, do we need the Content block? Couldn't everything inside the accordion block that isn't the trigger be treated as content? That's that tripped me off; if Content is the container, why couldn't I put inside multiple sections inside like you describe?

The Twentig plugin extends the Details block to create accordion options, and I was noticing that the trigger icons look a lot like the WP icon set

Those icons don't come from the default WP set, but they do look similar. We can create our own and make sure they match what's being displayed on the front-end. AFAIK, there's no easy way to display individual Gutenberg icons on the front-end, so we may need to use custom icons anyway.

Added a bunch to the Figma.

image

It's better for UX in my opinion

100% agree.

@jffng, one more thing: can we repeat some settings across the inner blocks and keep them in sync? For example, it'd be great to show the icon picker when the trigger block is selected. Currently, it's only displayed in the parent, which some folks may never discover. The same goes for the icon position.

for some reason when I try to export the Content block icon from Figma and load it into the block, I'm getting this:

Here's the SVG file! Let me know if it works.

content

jffng commented 1 month ago

Here's the SVG file! Let me know if it works.

Hmm that link is not allowing me to download it.

Nyiriland commented 1 month ago

Hmm that link is not allowing me to download it.

@jarekmorawski Can you "copy as svg" from Figma and paste in a code block (or point me to the icon in Figma)? There might be some issues with masking or layer order in the SVG code, I can help sort it out if needed.

jarekmorawski commented 1 month ago

Sure thing. FWIW, you can right-click on that SVG and download it like any other image. Here's the code:

`

`

jffng commented 1 month ago

Okay, I made the following changes:

A demo and plugin download is available at https://cool-tools.mystagingwebsite.com/2024/07/12/accordion/

https://cloudup.com/cBIxzqcM2Ym

One follow up for sure I want to do is to add a setting to adjust the icon size.

Nyiriland commented 1 month ago

Wow, looking awesome!!! Love all the options for the trigger ๐Ÿคก

I'm thinking about any "default styles" we might want to include (@jarekmorawski may have input here). To me, this is pretty much the default I'd want to go for:

image

To achieve this:

jarekmorawski commented 1 month ago

Changing styles: difficult to change on multiple accordions; you either have to copy styles, or duplicate one you've already styled

Global Styles is one option, multi-select is another. In the editor, you can select a few blocks of the same type to see and modify their settings. I'm not sure if that requires an extra API or something, but it's worth looking into.

To me, this is pretty much the default I'd want to go for:

100% agree. It'd be great to have a default style like this. In the future, we can add a few more as block styles.

On top of that, I have a few extra comments:

I don't think the empty option is working in the icon picker. It took me a second to understand what it was. We can either replace it with None (text button) or add a toggle Display icon that will display the picker when enabled.

image

It'd be nice if the newly added accordion block would replicate the recently used styles. Otherwise, we force the user to style it from scratch or resort to duplicating existing accordions.

image

I think the "Autoclose" label needs some rewording so it's more clear what it does (or at least change to "Auto-close"... I'll think on this)

I'd rename it to Close automatically and add a help text: Automatically close this accordion when visitors open another one.

H3 feels too big for a default style. Can we try H5?

image

If we are to add a toggle to disable animations, I'd put it Advanced so it doesn't get in most people's way.

Nyiriland commented 1 month ago

Good points on everything, @jarekmorawski.

We can either replace it with None (text button) or add a toggle Display icon that will display the picker when enabled.

I also didn't notice that there was a "none" option for the icon, hah. I'd vote for None as text, or remove the option altogether. (For a11y, there should be an indicator to imply additional functionality somewhere.)

H3 feels too big for a default style. Can we try H5?

I tested this out with 2024, and h5 feels too small (I'm stress testing over on this page, btw). h4 works better (although I am also still a fan of h3, too). @jffng is there any guidance around semantic hierarchy of DOM elements for block defaults? Like, I'd almost want to make it an h2, then size the text smaller, but that's probably a bad idea, haha. h4 probably is the way to go.

Another note...

image
Nyiriland commented 1 month ago

Another Q for @jffng... following up on the chat we had yesterday, I see there are focus styles applied at the block level, specifically to this class:

image

I previously thought these were inherited, but if they're not, can we remove it from the :focus and just keep on focus:visible, or does that need to stay to match button focus states? (I threw in a button on that test page and it actually doesn't match those, either.)

jffng commented 1 month ago

Can we add ability a border to the accordion group?

๐Ÿ‘

I'd vote for None as text

๐Ÿ‘

Automatically close this accordion when visitors open another one.

I find it a little confusing what accordion is being referred to by "this" and "another one" โ€”ย is it the accordion group or accordion item? Technically this is a setting of the group, but it affects the behavior of accordions inside the group. Also something to consider is when this setting is enabled and multiple accordion items are open by default, clicking any open section actually closes all of them.

What do you think about Clicking one accordion section automatically closes all other open sections.?

is there any guidance around semantic hierarchy of DOM elements for block defaults?

I think it depends what's on the page already and where the accordion appears (i.e. is there a h2 placed right before it that acts to describe / label the accordion?). Then h3 would be the correct level.

Also different themes and style variations are going to change the size of an h3/4/5. With that in mind, do you still think h4 or h5 is best for the default, or can we keep it h3?

can we remove it from the :focus and just keep on focus:visible

๐Ÿ‘

FYI I'm going to continue working on this over here, can we pick up the discussion / feedback there? ๐Ÿ™

https://github.com/WordPress/gutenberg/pull/64119/