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

feat(accordion): add content and toggle container props by redallen · Pull Request #2713 · patternfly/patternfly-react · GitHub

/ patternfly-react Public

feat(accordion): add content and toggle container props - #2713

Merged
tlabaj merged 7 commits into
patternfly:masterfrom
redallen:feat/accordion-containers
Sep 6, 2019
ConversationCommits (7)ChecksFiles changed
Merged

feat(accordion): add content and toggle container props#2713
tlabaj merged 7 commits into
patternfly:masterfrom
redallen:feat/accordion-containers

Conversation

redallen commented Aug 15, 2019

Copy link
Copy Markdown
Contributor

What: Fixes #2694

Additional issues:

patternfly-build commented Aug 15, 2019

Copy link
Copy Markdown
Collaborator

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

redallen requested review from jschuler and mturley August 15, 2019 19:14
redallen requested a review from boaz0 August 15, 2019 19:15

redallen commented Aug 15, 2019

Copy link
Copy Markdown
Contributor Author

Extends @boaz0 's work from #2422

boaz0 reviewed Aug 15, 2019
View reviewed changes

boaz0 left a comment

Copy link
Copy Markdown
Member

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

Just one comment but all in all LGTM 👍

Comment thread packages/patternfly-4/react-core/src/components/Accordion/Accordion.tsx Outdated
<AccordionList className={css(styles.accordion, className)} aria-label={ariaLabel} {...props}>
<AccordionContext.Provider value={{ AccordionHeadingLevel: headingLevel, asDefinitionList }}>
<AccordionContext.Provider value={{
HeadingLevel: headingLevel,

boaz0 Aug 15, 2019

Copy link
Copy Markdown
Member

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

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

redallen Aug 20, 2019

Copy link
Copy Markdown
Contributor Author

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

Fixed.

tlabaj requested a review from mcoker August 27, 2019 16:56
kmcfaul previously approved these changes Aug 29, 2019
View reviewed changes
mturley previously approved these changes Aug 30, 2019
View reviewed changes

mturley 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

Looks good!

tlabaj requested review from mcoker and removed request for mcoker September 4, 2019 19:07
tlabaj added the PF4 label Sep 4, 2019
mcoker previously approved these changes Sep 5, 2019
View reviewed changes

mcoker 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! 🥇

redallen dismissed stale reviews from mcoker, mturley, and kmcfaul via 1a79e31 September 6, 2019 13:57

redallen commented Sep 6, 2019

Copy link
Copy Markdown
Contributor Author

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

redallen force-pushed the feat/accordion-containers branch from 0596911 to c3a245f Compare September 6, 2019 14:31
kmcfaul previously approved these changes Sep 6, 2019
View reviewed changes
tlabaj suggested changes Sep 6, 2019
View reviewed changes
Comment thread packages/patternfly-4/react-core/src/components/Accordion/AccordionContent.tsx Outdated
isFixed?: boolean;
/** Adds accessible text to the Accordion content */
'aria-label'?: string;
/** Container to override the default for content */

tlabaj Sep 6, 2019

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

Maybe say Component to use as content container?

redallen Sep 6, 2019

Copy link
Copy Markdown
Contributor Author

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

Sure, done!

tlabaj approved these changes Sep 6, 2019
View reviewed changes

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

tlabaj merged commit 2c50a68 into patternfly:master Sep 6, 2019
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcoker mcoker left review comments
kmcfaul kmcfaul left review comments
jschuler Awaiting requested review from jschuler
+3 more reviewers
boaz0 boaz0 left review comments
mturley mturley approved these changes
tlabaj tlabaj approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

tlabaj

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Issues with Accordion styling

7 participants

Back | FazBrowse Home | New Git URL