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 content and toggle container props #2713

Merged
merged 7 commits into from Sep 6, 2019

Conversation

@redallen
Copy link
Contributor

commented Aug 15, 2019

What: Fixes #2694

Additional issues:

@patternfly-build

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

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

@redallen redallen requested review from mturley and jschuler Aug 15, 2019

@redallen redallen requested a review from boaz0 Aug 15, 2019

@redallen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Extends @boaz0 's work from #2422

@boaz0
Copy link
Member

left a comment

Just one comment but all in all LGTM 👍

@@ -27,7 +27,11 @@ export const Accordion: React.FunctionComponent<AccordionProps> = ({
const AccordionList: any = asDefinitionList ? 'dl' : 'div';
return (
<AccordionList className={css(styles.accordion, className)} aria-label={ariaLabel} {...props}>
<AccordionContext.Provider value={{ AccordionHeadingLevel: headingLevel, asDefinitionList }}>
<AccordionContext.Provider value={{
HeadingLevel: headingLevel,

This comment has been minimized.

Copy link
@boaz0

boaz0 Aug 15, 2019

Member

Do we need HeadingLevel other than in ToggleContainer because it looks like redundent

This comment has been minimized.

Copy link
@redallen

redallen Aug 20, 2019

Author Contributor

Fixed.

@tlabaj tlabaj added the css review label Aug 27, 2019

@tlabaj tlabaj requested a review from mcoker Aug 27, 2019

@mturley
Copy link
Collaborator

left a comment

Looks good!

@tlabaj tlabaj requested review from mcoker and removed request for mcoker Sep 4, 2019

@tlabaj tlabaj added the PF4 label Sep 4, 2019

@mcoker
Copy link
Contributor

left a comment

LGTM! 🥇

@redallen redallen dismissed stale reviews from mcoker, mturley, and kmcfaul via 1a79e31 Sep 6, 2019

@redallen redallen requested review from mturley, kmcfaul and tlabaj Sep 6, 2019

@redallen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

Can you please re-review the renamed prop @kmcfaul @tlabaj @mcoker @mturley . Thanks! 👍

@redallen redallen force-pushed the redallen:feat/accordion-containers branch from 0596911 to c3a245f Sep 6, 2019

@@ -16,6 +16,8 @@ export interface AccordionContentProps extends React.HTMLProps<HTMLDivElement> {
isFixed?: boolean;
/** Adds accessible text to the Accordion content */
'aria-label'?: string;
/** Container to override the default for content */

This comment has been minimized.

Copy link
@tlabaj

tlabaj Sep 6, 2019

Contributor

Maybe say Component to use as content container?

This comment has been minimized.

Copy link
@redallen

redallen Sep 6, 2019

Author Contributor

Sure, done!

@tlabaj
tlabaj approved these changes Sep 6, 2019
Copy link
Contributor

left a comment

LGTM

@mturley
mturley approved these changes Sep 6, 2019

@tlabaj tlabaj merged commit 2c50a68 into patternfly:master Sep 6, 2019

8 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
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.