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

feat(accordion): Add accordion to pf #1511

Merged
merged 8 commits into from Apr 23, 2019
Merged

Conversation

@ibolton336
Copy link
Member

ibolton336 commented Mar 6, 2019

What: closes #1387

Additional issues:

@ibolton336 ibolton336 added the PF4 label Mar 6, 2019
@ibolton336 ibolton336 requested a review from tlabaj Mar 6, 2019
@ibolton336 ibolton336 force-pushed the ibolton336:new-accordion branch 3 times, most recently from 3f7d31c to b108c4a Mar 6, 2019
@ibolton336 ibolton336 requested review from jschuler and rebeccaalpert Mar 6, 2019
@tlabaj tlabaj requested review from kmcfaul and removed request for tlabaj and rebeccaalpert Mar 6, 2019
@ibolton336 ibolton336 force-pushed the ibolton336:new-accordion branch 2 times, most recently from bc0e1e6 to 3eaad26 Mar 6, 2019
@redallen

This comment has been minimized.

Copy link
Contributor

redallen commented Mar 7, 2019

Rebase on latest master to fix CircleCI errors and get a deploy preview.

@ibolton336 ibolton336 force-pushed the ibolton336:new-accordion branch from 3eaad26 to 065c04c Mar 7, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 7, 2019

Codecov Report

Merging #1511 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1511      +/-   ##
==========================================
+ Coverage   82.74%   82.78%   +0.03%     
==========================================
  Files         601      608       +7     
  Lines        6647     6696      +49     
  Branches       72       76       +4     
==========================================
+ Hits         5500     5543      +43     
- Misses       1120     1123       +3     
- Partials       27       30       +3
Flag Coverage Δ
#patternfly3 84.87% <ø> (ø) ⬆️
#patternfly4 79.55% <100%> (+0.14%) ⬆️
#patternflymisc 95.68% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...4/react-core/src/components/Accordion/Accordion.js 100% <100%> (ø)
...t-core/src/components/Accordion/AccordionToggle.js 100% <100%> (ø)
...4/react-core/src/components/ChipGroup/ChipGroup.js 90.9% <0%> (-9.1%) ⬇️
...nfly-4/react-core/src/components/ChipGroup/Chip.js 75% <0%> (ø) ⬆️
...e/src/components/AboutModal/AboutModalBoxHeader.js 100% <0%> (ø) ⬆️
...atternfly-4/react-core/src/components/Card/Card.js 100% <0%> (ø) ⬆️
...y-4/react-table/src/components/Table/HeaderCell.js 100% <0%> (ø) ⬆️
...ly-4/react-core/src/components/Page/PageSection.js 100% <0%> (ø) ⬆️
...fly-4/react-table/src/components/Table/BodyCell.js 100% <0%> (ø) ⬆️
...react-core/src/components/AboutModal/AboutModal.js 70.83% <0%> (ø) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ced2b66...596b54c. Read the comment docs.

@tlabaj tlabaj requested a review from christiemolloy Mar 8, 2019
@ibolton336 ibolton336 force-pushed the ibolton336:new-accordion branch from 065c04c to 245bfd6 Mar 11, 2019
@ibolton336 ibolton336 force-pushed the ibolton336:new-accordion branch from 245bfd6 to 23baadf Mar 21, 2019
@redallen

This comment has been minimized.

Copy link
Contributor

redallen commented Mar 29, 2019

CircleCI times out before it shows this, but there is this error that needs to be fixed that happens when building the docs locally @ibolton336 :

@patternfly/react-docs: > 1 | export var matches = Element.prototype.matches || Element.prototype.msMatchesSelector || Element.prototype.webkitMatchesSelector;
@patternfly/react-docs:     |                      ^
@patternfly/react-docs:   2 | 
@patternfly/react-docs:   3 | if (!Element.prototype.matches) {
@patternfly/react-docs:   4 |   Element.prototype.matches = matches;
@patternfly/react-docs: 
@patternfly/react-docs:   WebpackError: ReferenceError: Element is not defined
@patternfly/react-docs:   
@patternfly/react-docs:   - closestPolyfill.js:1 Module.../../patternfly-3/patternfly-react/dist/esm/com    mon/closestPolyfill.js
@patternfly/react-docs:     lib/Users/zallen/src/patternfly-react-master/packages/patternfly-3/patternfl    y-react/dist/esm/common/closestPolyfill.js:1:22

Run yarn build:docs locally to reproduce. Let me know if everything's fine on your end and I'll take another look!

@ibolton336 ibolton336 force-pushed the ibolton336:new-accordion branch from b27a356 to 111baa6 Apr 1, 2019
Copy link
Contributor

tlabaj left a comment

@ibolton336 can you add the accordian wrapper to your PR? you will need to wait until master has updated to a later version of core.
.pf-c-accordion__expanded-content-body (child of .pf-c-accordion__expanded-content)

This is a change from core referenced in issue #1682

Copy link
Contributor

tlabaj left a comment

@ibolton336 The way the docs are being generated was recently changed, can you please rebase and move the examples into the MD file.

@ibolton336 ibolton336 force-pushed the ibolton336:new-accordion branch 2 times, most recently from b3028ff to 9751033 Apr 18, 2019
@ibolton336 ibolton336 force-pushed the ibolton336:new-accordion branch from 9751033 to bdce0b3 Apr 18, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Apr 22, 2019

redallen added 2 commits Apr 22, 2019
@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Apr 22, 2019

I don't think that the behavior is quite right here. In a classic accordion control, only one panel is ever opened at a time. See the behavior of the PF3 Accordion component, for example: https://www.patternfly.org/pattern-library/widgets/#accordion Note here that opening a new panel closes the current one. That said, it could be useful to have both variations - one with simple collapsible panels as you have now and another withe the accordion behavior as described above.

The other thing I questioned was the difference between the Simple and the Fixed accordion. My understanding is that the intent of the fixed accordion is that the height is constrained. So if the contents will not fit within the max height, a scrollbar will be exposed within the open panel to scroll the content. Is that also what you intended @mcoker ?

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Apr 22, 2019

@mcarrano Agreed re: having the option for both types of behavior.

Re: the fixed examples, yep - looking at the pf3 fixed height accordion, this accordion is the same where the max height is defined on the expandable panel for an accordion item. In the PF3 examples, that max-height is 69px. In the PF4 component, looks like we went with 150px.

Also the .pf-c-accordion__toggle button shouldn't be a PF4 <Button> component (currently .pf-c-button.pf-m-plain), it should just use a regular <button> html element.

And when an item is expanded, we also need to add .pf-m-expanded to the expanded dd.

@tlabaj

This comment has been minimized.

Copy link
Contributor

tlabaj commented Apr 23, 2019

@mcarrano @mcoker We are going to merge this and make the updates in a new PR.

@jgiardino

This comment has been minimized.

Copy link
Collaborator

jgiardino commented Apr 23, 2019

Some initial comments after a quick review...

aria-label shouldn't be a required prop. Also, it makes sense that it's included on the <dl> so I will add an issue to track that.

The section headings that are expand/collapse buttons have aria-label="Details" and also aria-labelledby="..." defined. These attributes should not be there. This overrides the actual heading text that's specified, preventing that text from being announced.

Core does not include aria-controls. This attribute is noted as optional in the WAI-ARIA practices for disclosure (show/hide). I would suggest leaving it out, since the controlled contents are immediately after the button.

redallen and others added 2 commits Apr 23, 2019
Jessie
@tlabaj
tlabaj approved these changes Apr 23, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@redallen redallen merged commit c02e683 into patternfly:master Apr 23, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.