FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(accordion): Introduce accordion by jessiehuff · Pull Request #1852 · patternfly/patternfly-react · GitHub

feat(accordion): Introduce accordion - #1852

Merged
redallen merged 16 commits into
patternfly:masterfrom
jessiehuff:new-accordion
Apr 26, 2019
Merged

feat(accordion): Introduce accordion#1852
redallen merged 16 commits into
patternfly:masterfrom
jessiehuff:new-accordion

Conversation

jessiehuff commented Apr 25, 2019
edited
Loading

Copy link
Copy Markdown
Contributor

What: Resolves #1387

Additional issues:

jessiehuff changed the title New accordion Introduce Accordion Component Apr 25, 2019

Copy link
Copy Markdown
Collaborator

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

jessiehuff changed the title Introduce Accordion Component feat(accordion): Introduce accordion Apr 25, 2019

codecov-io commented Apr 25, 2019
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1852 into master will decrease coverage by 0.24%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1852      +/-   ##
==========================================
- Coverage   82.84%   82.59%   -0.25%     
==========================================
  Files         612      621       +9     
  Lines        6743     6844     +101     
  Branches       82       93      +11     
==========================================
+ Hits         5586     5653      +67     
- Misses       1126     1151      +25     
- Partials       31       40       +9
Flag Coverage Δ
#patternfly3 84.87% <ø> (ø) ⬆️
#patternfly4 79.29% <ø> (-0.46%) ⬇️
#patternflymisc 95.68% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...t-core/src/components/Accordion/AccordionToggle.js 100% <ø> (ø) ⬆️
...4/react-core/src/components/Accordion/Accordion.js 100% <ø> (ø) ⬆️
...react-core/src/components/Wizard/WizardNavItem.tsx 50% <0%> (-26.48%) ⬇️
...-4/react-core/src/components/Wizard/WizardBody.tsx 60% <0%> (-20%) ⬇️
...rnfly-4/react-core/src/components/Select/Select.js 73.52% <0%> (-19.81%) ⬇️
.../react-core/src/components/Wizard/WizardToggle.tsx 58.06% <0%> (-3.23%) ⬇️
...nfly-4/react-core/src/components/Wizard/Wizard.tsx 58.42% <0%> (-2.95%) ⬇️
.../react-core/src/components/Wizard/WizardHeader.tsx 93.33% <0%> (ø) ⬆️
...y-4/react-core/src/components/Wizard/WizardNav.tsx 92.3% <0%> (ø) ⬆️
...eact-core/src/components/Select/selectConstants.js 100% <0%> (ø) ⬆️
... and 13 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 0a47e4e...75f64d0. Read the comment docs.

dlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Tests look like they are failing and dist not building

mcoker commented Apr 26, 2019

Copy link
Copy Markdown
Contributor

Not sure if this is already on the list, but we also need to add .pf-m-expanded to the expanded dd, so that there is a blue line to the left of the button/toggle and expanded content, like this

Copy link
Copy Markdown
Contributor

Looks good, @jessiehuff! I just have a couple of minor comments about things to remove.

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

Related to this, the <AccordionToggle> should not have the prop aria-label included.

The examples include aria-label="Primary Content Details" on the <AccordionContent> components. We can keep this as a prop on the component, but to match core we should remove this prop in our examples.

dlabaj previously approved these changes Apr 26, 2019

dlabaj commented Apr 26, 2019

Copy link
Copy Markdown
Contributor

Looks Great Thanks @jessiehuff !

tlabaj added PF3 and removed PF3 labels Apr 26, 2019
tlabaj previously approved these changes Apr 26, 2019

tlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

jessiehuff dismissed stale reviews from tlabaj and dlabaj via 75f64d0 April 26, 2019 19:32

jgiardino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Great job on this!

redallen merged commit 4ae95e9 into patternfly:master Apr 26, 2019

Copy link
Copy Markdown
Contributor

Error in import #1869 we can't upgrade :(

node_modules/@patternfly/react-core/dist/js/components/Accordion/Accordion.d.ts
(2,22): Cannot find module '../../typeUtils'

import { Omit } from '../../typeUtils';

Fix with

import { Omit } from '../../helpers/typeUtils';

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Introduce the Accordion Component

9 participants


Back | FazBrowse Home | New Git URL