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

Add dedicated accordion component #32013

Merged
merged 6 commits into from
Nov 6, 2020
Merged

Add dedicated accordion component #32013

merged 6 commits into from
Nov 6, 2020

Conversation

mdo
Copy link
Member

@mdo mdo commented Oct 30, 2020

This PR replaces and adds onto #30042. The goal of this is to replace our half-baked card accordion with a fully functioning accordion component, powered by the Collapse plugin. While this adds some CSS, personally I think it makes accordions more widely accessible (e.g., fewer requirements to creating the component).

According to the origin PR by @gijsroge, this does the following:

Also from that PR, there were some remaining todos. Design and code are largely final now with my latest changes here. Looking through https://www.w3.org/TR/wai-aria-practices-1.1/#accordion, I think we're largely in order. There's also this handy dandy example accordion they provide.

Preview: https://deploy-preview-32013--twbs-bootstrap.netlify.app/docs/5.0/components/accordion/


Some open questions from me now moving this forward:

  • Do we need striping here? I think not, and may remove it shortly.
  • Any other variations, documentation, etc we should be considering?

/cc @twbs/css-review

@mdo mdo requested a review from a team as a code owner October 30, 2020 04:07
@XhmikosR

This comment has been minimized.

@mdo mdo force-pushed the v5-accordion branch 2 times, most recently from 05f306b to 7e9b5db Compare November 2, 2020 19:39
@MartijnCuppens
Copy link
Member

The bottom border is missing when the last element is collapsing
image

The bottom border is below the focus state here:
image

@patrickhlauke
Copy link
Member

Just gave this a quick check in terms of accessibility - of course, as it's based on our current one, it's all kosher.

One small request though: would it be possible to ditch the latin placeholder text for some generic english text? Even if it's just something that is self explanatory, like "This is the example content for the accordion panel. It is revealed when the accordion control is activated." or similar? It's not a huge deal, but it avoids having the page all of a sudden switch to absolutely incomprehensible gibberish for screen reader users, for instance.

@MartijnCuppens
Copy link
Member

Fixed the remaining bugs by adding an additional accordion-collapse class.

@mdo
Copy link
Member Author

mdo commented Nov 5, 2020

Fixed the remaining bugs by adding an additional accordion-collapse class.

I... think I love you? Thank you for the fixes!

@XhmikosR
Copy link
Member

XhmikosR commented Nov 6, 2020

Saved a few more bytes by reordering the SVG attributes :)

mdo and others added 6 commits November 6, 2020 14:45
Switches to slightly more verbose, but more consistent, z-index layering we use elsewhere (e.g., pagination). Doing this ensures we're not toggling position on and off, but rather z-index.
This results in slightly better gzip compression
@XhmikosR XhmikosR merged commit bb6daab into main Nov 6, 2020
@XhmikosR XhmikosR deleted the v5-accordion branch November 6, 2020 12:49
@patrickhlauke
Copy link
Member

Fixed the remaining bugs by adding an additional accordion-collapse class.

oh, i just noticed that includes also changing the latin text. thank you, top stuff! (funnily enough, I've just written up a bug for another project that also uses lorem ipsum for their demos :) )

@XhmikosR XhmikosR changed the title v5: Accordion, Take 2 Add dedicated accordion component Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants