Skip to content
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

Implement Accordion Block Plugin #16

Open
wants to merge 77 commits into
base: trunk
Choose a base branch
from
Open

Conversation

jffng
Copy link

@jffng jffng commented Jul 19, 2024

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:

  • Accordion
    • Accordion Item
      • Accordion Trigger
      • Accordion Content

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

  • Allows you to configure which items are open by default, and the expand / collapse behavior
  • Comes with a few block styles for the icons
  • Informed by design mockups by @jarekmorawski
  • Uses the Interactivity API
  • Meets WC3 accessibility guidelines

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

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

@Nyiriland
Copy link

Nyiriland commented Jul 22, 2024

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
Copy link

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
Copy link

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
Copy link
Author

jffng commented Jul 23, 2024

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

Hmm that link is not allowing me to download it.

@Nyiriland
Copy link

Nyiriland commented Jul 23, 2024

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
Copy link

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

<svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg"> <path fill-rule="evenodd" clip-rule="evenodd" d="M8.10417 6.00024H6.5C5.39543 6.00024 4.5 6.89567 4.5 8.00024V10.3336H6V8.00024C6 7.7241 6.22386 7.50024 6.5 7.50024H8.10417V6.00024ZM4.5 13.6669V16.0002C4.5 17.1048 5.39543 18.0002 6.5 18.0002H8.10417V16.5002H6.5C6.22386 16.5002 6 16.2764 6 16.0002V13.6669H4.5ZM10.3958 6.00024V7.50024H13.6042V6.00024H10.3958ZM15.8958 6.00024V7.50024H17.5C17.7761 7.50024 18 7.7241 18 8.00024V10.3336H19.5V8.00024C19.5 6.89567 18.6046 6.00024 17.5 6.00024H15.8958ZM19.5 13.6669H18V16.0002C18 16.2764 17.7761 16.5002 17.5 16.5002H15.8958V18.0002H17.5C18.6046 18.0002 19.5 17.1048 19.5 16.0002V13.6669ZM13.6042 18.0002V16.5002H10.3958V18.0002H13.6042Z" fill="#1E1E1E"/> </svg>

@jffng
Copy link
Author

jffng commented Jul 30, 2024

Okay, I made the following changes:

  • Matched the editing experience to the front-end so that accordion content is only visible if the block is selected or it's set to be open by default
  • Fixed the padding issue on the Content, shoutout @Nyiriland!!
  • Updated the UI icons, so they are now in the block settings of the Trigger. I'm not sure about repeating settings / keeping them in sync across blocks, is there an example of this you have seen? Happy to explore in a follow up.
  • Renamed the blocks to Accordion Group > Accordion Item > Trigger + Content
  • Updated the block icons
  • Renamed the "Allow multiple items open" setting to "Autoclose", and it's off by default

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
Copy link

Wow, looking awesome!!! Love all the options for the trigger 🤡

  • Changing styles: difficult to change on multiple accordions; you either have to copy styles, or duplicate one you've already styled
  • Let's look into if it's possible to register blocks in "global styles" to make this ^ easier
  • Can we add ability a border to the accordion group? Specifically, if you have a bottom border on all your accordions, you should be able to add a border to the top of the group: this will allow you to re-order the child elements, while still ensuring there's always a top border.
  • I know this may be a theme-level style change, but changing the focus style on the trigger to focus-visible would make that irritating blue border disappear when you click it.
  • Could we have an option to turn off the open/close animation transition on the group? (But keep it on by default)
  • 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'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:

  • Zero block spacing on accordion group
  • 1px border-top on accordion group
  • 1px border-bottom on accordions
  • spacing 2 on top & bottom of accordion trigger
  • spacing 2 on bottom of content

@jarekmorawski
Copy link

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
Copy link

Nyiriland commented Aug 1, 2024

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...

  • Gap on trigger: I previously hadn't tested with long text in the trigger, but we should add a gap (like .75em) between the icon and tigger text:
image

@Nyiriland
Copy link

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
Copy link
Author

jffng commented Aug 6, 2024

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? 🙏

WordPress/gutenberg#64119

@jffng jffng mentioned this pull request Aug 12, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Review 👓
Development

Successfully merging this pull request may close these issues.

3 participants