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

Codecov Report

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

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

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.

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!

PatternFly-React preview: https://1511-pr-patternfly-react-patternfly.surge.sh

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 ?

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

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

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.