Accordion by christiemolloy · Pull Request #1357 · patternfly/patternfly

christiemolloy · 2019-01-31T20:35:23Z

Closes #116.

maryshak1996 · 2019-01-31T20:48:25Z

@christiemolloy do you have a link for it that I could take a look at? :)

christiemolloy · 2019-01-31T21:17:25Z

@maryshak1996 just bumped the a11y count, hopefully it will pass!

patternfly-build · 2019-02-01T14:25:04Z

Deploy preview for pf-next ready!

Built with commit c51ad9b

https://deploy-preview-1357--pf-next.netlify.com

mcarrano

Looks good to me!

jgiardino · 2019-02-04T16:38:33Z

Great job with this one!

For comparison, I looked at these examples:

https://www.w3.org/TR/wai-aria-practices-1.1/examples/accordion/accordion.html
- this one is kind of similar to your implementation, in that they use a <dl> where you use a <ul>. Since the panel contents can be anything, using a <dd> seems a little better suited than putting them inside the <li>.
https://inclusive-components.design/collapsible-sections/#state
- I kind of like the simplicity of differentiating the sections using headings. For example, if we didn't have the expand/collapse feature, these contents would just be sections separated by headings.
- The author of this one makes some interesting points about aria-controls not being needed in this type of component. His comments are consistent with feedback we received when testing, that if the controlled contents immediately follow the toggle, then they know by proximity that they're related.

Let me know what you think about these options. We can also bring this up during the a11y review meeting to let others weigh in.

jgiardino · 2019-02-04T16:40:15Z

src/patternfly/components/Accordion/accordion-item.hbs

@@ -0,0 +1,6 @@
+<li class="pf-c-accordion__item{{#if accordion-item--IsExpanded}} pf-m-expanded{{/if}}{{#if accordion-item--modifier}} {{accordion-item--modifier}}{{/if}}"


The aria-expanded attribute should always be placed on the interactive element. So in this case, it should be moved from the <li> to the button.pf-c-accordion__item-button.

jgiardino · 2019-02-04T16:44:17Z

src/patternfly/components/Accordion/examples/accordion-fluid-example.hbs

@@ -0,0 +1,41 @@
+{{#> accordion}}
+  {{#> accordion-item accordion-item--attribute='aria-expanded="false"'}}
+    {{#> accordion-item-button accordion-item-button--attribute='aria-controls="ex-content-1"'}}


I know this is an attribute that's suggested in the wai-aria example for Accordion, but I lean toward only using this attribute for cases where the controlled contents do not immediately follow the toggle. I'd suggest to just remove this attribute altogether.

maryshak1996 · 2019-02-04T16:59:40Z

@christiemolloy this came out great!

christiemolloy · 2019-02-04T19:07:32Z

@jgiardino thanks for your comments... I made the respective updates. I agree with everything you mentioned, so its up to you if you want to bring them up in the a11y meeting - but I don't think they're too controversial.

matthewcarleton

This is looking solid Christie.
Was there any discussion what the buttons would look like with longer strings of text? Right now it just breaks to a second line.

matthewcarleton · 2019-02-05T13:45:20Z

src/patternfly/components/Accordion/accordion.hbs

@@ -0,0 +1,6 @@
+<ul class="pf-c-accordion{{#if accordion--modifier}} {{accordion--modifier}}{{/if}}"


Should this be a <dl>

The wai-aria example uses <dl> but when I test their implementation in VO, I don't get any of the semantics associated with <dl>. I'm not sure if that's because they change the role of <dt> to a heading or not, but doing that seems to render the use of <dl> kind of pointless.

This implementation is also missing the <dt> element, and it might be worth checking in a screen reader what happens when you nest all the toggle elements like this:
<dt><h2><button class="pf-c-accordion__item-button" aria-expanded="false">...
instead of follow what wai-aria does with
<dt role="heading" aria-level="3"><button aria-expanded="true" ...>...

But I think it would also be fine just use <div>s for both .pf-c-accordion and .pf-c-accordion__item in this implementation, especially if the nesting suggestion above doesn't produce anything more meaningful in the screen reader experience.

ya I was wondering why we couldn't use <dt> in place of the <h2> if we are going with the <dl>, <dd> structure

I updated this to use <dl> > <div> > <dt> > <dd> We have the extra div because of the blue border on the left, but I could try rearranging the CSS to style this border without?

ya you might be able to just style the border of the <dt> and the expanded <dd> rather than having the wrapping div. 🤔

.pf-c-accordion-item as a <div> wrapper takes on accordion-item--IsExpanded and unassigns hidden to .pf-c-accordion-item-expanded-content. But that is because it is the parent of .pf-c-accordion-item-expanded-content . Without this div wrapper, .pf-c-accordion-item__button sits adjacent to .pf-c-accordion-item__expanded-contentand because they are siblings the expanded assignment doesn't work. However the wai-aria example uses js to toggle which item is hidden. So it seems to me that it needs this parent wrapper div to handle the expanded content in CSS. @matthewcarleton

matthewcarleton · 2019-02-05T13:46:38Z

src/patternfly/components/Accordion/accordion.scss

+
+.pf-c-accordion {
+  // accordion
+  --pf-c-accordion--Width: #{pf-size-prem(300px)};


I know this is coming from the design but are we saying that accordions couldn't be a different width? It would seem more likely that they would set the accordion width in the layout. What do you think?

I agree. Not sure we can always predict the width of an accordion as it would be driven by layout and content.

I was following the design, but I definitely agree that its layout should handle its width.

matthewcarleton · 2019-02-05T15:18:20Z

src/patternfly/components/Accordion/accordion.scss

+  --pf-c-accordion--BackgroundColor: var(--pf-global--BackgroundColor--light-100);
+  --pf-c-accordion--BorderWidth: var(--pf-global--BorderWidth--sm);
+  --pf-c-accordion--BoxShadow: var(--pf-global--BoxShadow--md);
+  --pf-c-accordion--PaddingTop: var(--pf-global--spacer--lg);


Looks like the design is calling for 32px here and the bottom.

matthewcarleton · 2019-02-05T16:00:49Z

src/patternfly/components/Accordion/accordion.scss

+  .pf-c-accordion__item {
+    position: relative;
+
+    &::before {


Have you tried writing this so you aren't applying all these to every item when they are aren't expanded?

I had seen this done in data list, and I assumed this way would avoid the accordion item jittering when it expanded... but when I write this the other way it makes no difference. I guess we should be consistent with this across the all of pf - what do you recommend @matthewcarleton

I'd say it's a more efficient way to write it, but maybe test it first to make sure 🤷‍♂️

Okay it doesnt make a difference to this component so im in favor of the more efficient way :)

src/patternfly/components/Accordion/accordion.scss

matthewcarleton · 2019-02-05T17:56:26Z

src/patternfly/components/Accordion/docs/code.md

+| -- | -- | -- |
+| `aria-expanded="false"` | `.pf-c-accordion__item` | Indicates that the expanded content element is hidden. **Required**|
+| `aria-expanded="true"` | `.pf-c-accordion__item`, `.pf-c-accordion__item-expanded-content` | Indicates that the expanded content element is visible. **Required**|
+| `hidden` | `.pf-c-accordion__item-expanded-content` | Indicates that the expanded content element is hidden.**Required** |


should we clarify that this should be used with aria-expanded="false"?

matthewcarleton · 2019-02-05T17:57:11Z

src/patternfly/components/Accordion/docs/code.md

+
+| Class | Applied To | Outcome |
+| -- | -- | -- |
+| `.pf-c-accordion` | `<ul>` | Initiates an accordion component. **Required**|


I think this is going to be a <dl>

Can you update this to a <dl>

matthewcarleton · 2019-02-05T17:57:25Z

src/patternfly/components/Accordion/docs/code.md

+| Class | Applied To | Outcome |
+| -- | -- | -- |
+| `.pf-c-accordion` | `<ul>` | Initiates an accordion component. **Required**|
+| `.pf-c-accordion__item` | `<li>` | Initiates a list item in the accordion component. **Required** |


I think this should be a <dt>

src/patternfly/components/Accordion/docs/code.md

christiemolloy · 2019-02-06T15:14:32Z

@matthewcarleton left some comments - also when the buttons reach a longer length the string truncates at a width of 90% - what do you think about that, at a width of 100% it runs into the toggle arrow.

matthewcarleton · 2019-02-07T18:13:50Z

@christiemolloy I think that works. The only issue I see is when it's much wider 90% leaves quite a bit of space. That may be irrelevant though. I think this could actually work in the dropdown too, but would have to test to see. nice!

matthewcarleton

Hey Christie, this is looking really close. I made a few final comments. The only other thing I wonder about is how it's documented. We don't document the <dt> or the <h3> but both are required either for semantic reasons of accessibilities reasons. Do you think they should be added?

matthewcarleton · 2019-02-12T08:14:22Z

src/patternfly/components/Accordion/accordion.scss

+  // This component always needs to be light
+  @include pf-t-light;
+
+  position: relative;


this can be removed

matthewcarleton · 2019-02-12T08:15:46Z

src/patternfly/components/Accordion/accordion.scss

+    .pf-c-accordion__button-text {
+      width: var(--pf-c-accordion__button-text--width);
+      overflow: hidden;
+      text-align: left;


this can be removed

when I remove this it centers the content - its because of the width placed on the text when it truncates. so changed width to max-width

matthewcarleton · 2019-02-12T08:18:09Z

src/patternfly/components/Accordion/accordion.scss

+  --pf-c-accordion__button--hover--BackgroundColor: var(--pf-global--BackgroundColor--150);
+  --pf-c-accordion__button--focus--BackgroundColor: var(--pf-global--BackgroundColor--150);
+  --pf-c-accordion__button--active--BackgroundColor: var(--pf-global--BackgroundColor--150);
+  --pf-c-accordion__button--m-expanded--before--ZIndex: var(--pf-global--ZIndex--xl);


this can be removed

matthewcarleton · 2019-02-12T08:22:33Z

src/patternfly/components/Accordion/docs/code.md

+
+| Class | Applied To | Outcome |
+| -- | -- | -- |
+| `.pf-c-accordion` | `<ul>` | Initiates an accordion component. **Required**|


Can you update this to a <dl>

matthewcarleton · 2019-02-12T08:24:02Z

src/patternfly/components/Accordion/docs/code.md

+| `.pf-c-accordion__button-text` | `<span>` | Initiates the text inside the button. **Required** |
+| `.pf-c-accordion__button-toggle` | `<span>` | Initiates the toggle inside the button. **Required** |
+| `.pf-c-accordion__button-toggle-icon` | `<i>` | Initiates the toggle icon inside the toggle. **Required** |
+| `.pf-c-accordion__expanded-content` | `<div>` | Initiates an expanded content. **Must be paired with a button** |


This should be a <dd>

matthewcarleton · 2019-02-12T10:06:01Z

src/patternfly/components/Accordion/accordion-expanded-content.hbs

+  {{#if accordion-expanded-content--attribute}}
+    {{{accordion-expanded-content--attribute}}}
+  {{/if}}>
+  Lorem ipsum dolor sit amet, consectetur adipiscing elit. Duis molestie lorem lacinia dolor aliquet faucibus. Suspendisse gravida imperdiet accumsan. Aenean auctor lorem justo, vitae tincidunt enim blandit vel. Aenean quis tempus dolor. Lorem ipsum dolor sit amet, consectetur adipiscing elit Bla Bla Blee Bla. 


can you move this out of here and into the examples. That way you can add extra content for the fixed example to show how it scrolls.

matthewcarleton · 2019-02-12T10:06:50Z

src/patternfly/components/Accordion/accordion-item.hbs

@@ -0,0 +1,6 @@
+<div class="pf-c-accordion__item{{#if accordion-item--IsExpanded}} pf-m-expanded{{/if}}{{#if accordion-item--modifier}} {{accordion-item--modifier}}{{/if}}"


you can remove this file.

matthewcarleton · 2019-02-12T10:16:06Z

src/patternfly/components/Accordion/accordion-button-toggle.hbs

@@ -0,0 +1,7 @@
+<span class="pf-c-accordion__button-toggle{{#if accordion__button-toggle--modifier}} {{accordion__button-toggle--modifier}}{{/if}}"


Is there any reason why you can't just have the .pf-c-accordion__button-toggle-icon ? I'd say we can take the same approach here as we do in the dropdown where we just use an icon and not need the wrapper.

yep you're right!

christiemolloy · 2019-02-12T15:07:05Z

thanks @matthewcarleton made updates!

christiemolloy added 6 commits January 30, 2019 15:51

feat(accordion): add accordion component

5d40958

updates

c25e1f1

update

c443d51

updates

10c6995

updates

ec53291

updates

e0eef36

christiemolloy added the Feature label Jan 31, 2019

christiemolloy requested review from jgiardino, maryshak1996, matthewcarleton and mcoker January 31, 2019 20:35

christiemolloy added 2 commits January 31, 2019 16:14

update a11y

5b14a99

mc

4350a64

christiemolloy requested a review from mcarrano February 1, 2019 21:08

mcarrano approved these changes Feb 1, 2019

View reviewed changes

jgiardino reviewed Feb 4, 2019

View reviewed changes

jenns updates

fab25c1

matthewcarleton reviewed Feb 5, 2019

View reviewed changes

christiemolloy added 2 commits February 6, 2019 09:58

updates

44d82a9

change text to wrap

732c8c7

updates

7bd6ab8

christiemolloy added 5 commits February 8, 2019 14:13

remove div wrapper

62c1fcb

updates

c000e83

updates

f333e94

Merge branch 'master' into accordian

56e427f

updates

5590610

matthewcarleton reviewed Feb 12, 2019

View reviewed changes

updates

18beb8f

updates

c51ad9b

matthewcarleton merged commit ecd2d69 into patternfly:master Feb 14, 2019

christiemolloy mentioned this pull request Feb 14, 2019

Introduce the Accordion Component patternfly/patternfly-react#1387

Closed

mcoker mentioned this pull request Oct 20, 2023

feat(Accordion): added left-aligned toggle variant #5987

Merged

		@@ -0,0 +1,6 @@
		<li class="pf-c-accordion__item{{#if accordion-item--IsExpanded}} pf-m-expanded{{/if}}{{#if accordion-item--modifier}} {{accordion-item--modifier}}{{/if}}"

		@@ -0,0 +1,6 @@
		<ul class="pf-c-accordion{{#if accordion--modifier}} {{accordion--modifier}}{{/if}}"

		@@ -0,0 +1,6 @@
		<div class="pf-c-accordion__item{{#if accordion-item--IsExpanded}} pf-m-expanded{{/if}}{{#if accordion-item--modifier}} {{accordion-item--modifier}}{{/if}}"

		@@ -0,0 +1,7 @@
		<span class="pf-c-accordion__button-toggle{{#if accordion__button-toggle--modifier}} {{accordion__button-toggle--modifier}}{{/if}}"

Conversation

christiemolloy commented Jan 31, 2019

Uh oh!

maryshak1996 commented Jan 31, 2019

Uh oh!

christiemolloy commented Jan 31, 2019

Uh oh!

patternfly-build commented Feb 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

jgiardino commented Feb 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maryshak1996 commented Feb 4, 2019

Uh oh!

christiemolloy commented Feb 4, 2019

Uh oh!

matthewcarleton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewcarleton Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

christiemolloy commented Feb 6, 2019

Uh oh!

matthewcarleton commented Feb 7, 2019

Uh oh!

matthewcarleton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Feb 1, 2019 •

edited

Loading

matthewcarleton Feb 5, 2019 •

edited

Loading

christiemolloy Feb 12, 2019 •

edited

Loading