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): Introduce accordion #1852

Merged
merged 16 commits into from Apr 26, 2019
Merged

Conversation

@jessiehuff
Copy link
Contributor

jessiehuff commented Apr 25, 2019

What: Resolves #1387

Additional issues:

@jessiehuff jessiehuff changed the title New accordion Introduce Accordion Component Apr 25, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Apr 25, 2019

@jessiehuff jessiehuff force-pushed the jessiehuff:new-accordion branch from c92314c to 9545f9d Apr 25, 2019
@jessiehuff jessiehuff changed the title Introduce Accordion Component feat(accordion): Introduce accordion Apr 25, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 25, 2019

Codecov Report

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

Impacted file tree graph

@@            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 dlabaj requested review from ibolton336, jgiardino, dlabaj and tlabaj Apr 26, 2019
Copy link
Contributor

dlabaj left a comment

Tests look like they are failing and dist not building

Jessie added 3 commits Apr 26, 2019
Jessie
Jessie
Jessie
@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Apr 26, 2019

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

Screen Shot 2019-04-26 at 8 43 53 AM

@jgiardino

This comment has been minimized.

Copy link
Collaborator

jgiardino commented Apr 26, 2019

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.

Jessie
Jessie added 2 commits Apr 26, 2019
Jessie
@dlabaj

This comment has been minimized.

Copy link
Contributor

dlabaj commented Apr 26, 2019

Looks Great Thanks @jessiehuff !

@tlabaj tlabaj added PF3 PF4 enhancement 🚀 and removed PF3 labels Apr 26, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

Jessie
@jessiehuff jessiehuff dismissed stale reviews from tlabaj and dlabaj via 75f64d0 Apr 26, 2019
Copy link
Collaborator

jgiardino left a comment

Great job on this!

@dlabaj
dlabaj approved these changes Apr 26, 2019
@redallen redallen merged commit 4ae95e9 into patternfly:master Apr 26, 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
@aljesusg

This comment has been minimized.

Copy link
Contributor

aljesusg commented Apr 29, 2019

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';
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.