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(chipGroup): add heading level prop to toolbar chip group label #2278

Merged
merged 1 commit into from Jun 27, 2019

Conversation

@boaz0
Copy link
Member

boaz0 commented Jun 17, 2019

What:

closes #2167

  • add heading level prop to ChipGroup
  • pass heading level to Chip Group Toolbar item through Context API
  • update tests
  • HeadingLevel type was moved from Title to helpers
  • Chip Group Toolbar Item is using HeadingLevel type in its own heading level prop
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 17, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 17, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@a5d571e). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #2278   +/-   ##
========================================
  Coverage          ?   80.6%           
========================================
  Files             ?     666           
  Lines             ?    8441           
  Branches          ?     720           
========================================
  Hits              ?    6804           
  Misses            ?    1274           
  Partials          ?     363
Flag Coverage Δ
#patternfly3 85.23% <ø> (?)
#patternfly4 76.18% <0%> (?)
#patternflymisc 95.79% <ø> (?)
Impacted Files Coverage Δ
...ernfly-4/react-core/src/components/Title/Title.tsx 94.11% <ø> (ø)
.../src/components/ChipGroup/ChipGroupToolbarItem.tsx 50% <0%> (ø)

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 a5d571e...65e2fde. Read the comment docs.

@boaz0 boaz0 force-pushed the boaz0:closes_2167 branch from 07ded6a to c18fd16 Jun 17, 2019
@boaz0 boaz0 force-pushed the boaz0:closes_2167 branch from c18fd16 to 8c69ad6 Jun 21, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 21, 2019

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

@jgiardino

This comment has been minimized.

Copy link
Collaborator

jgiardino commented Jun 21, 2019

The prop works as expected! 🎉

The only question I have is whether the prop should move to the parent <ChipGroup> component?

With the Accordion component, we discussed this and ended up supporting the prop on the parent <Accordion> component, because in that component, it would be really odd to have different heading levels for different toggles (because there would then be a disconnect between the visual presentation and the semantic structure).

I would think that same reason is true in this case, but am curious if anyone has some reason for supporting this prop on the <ChipGroupToolbarItem> component instead.

I'm pulling in @mturley because he had provided suggestions on how to implement this for Accordion, and might have relevant info that would be helpful to share for this case as well.

@boaz0 boaz0 force-pushed the boaz0:closes_2167 branch from 8c69ad6 to 16f6e4a Jun 25, 2019
@mturley

This comment has been minimized.

Copy link
Collaborator

mturley commented Jun 25, 2019

@boaz0, I agree with @jgiardino that we should probably have the consumer pass one headingLevel prop to the parent <ChipGroup> instead of having to pass it to each <ChipGroupToolbarItem>, since it reduces duplication in the consumer's code and prevents them from using different heading levels in the same chip group.

@jenny-s51 implemented this for Accordion using React's Context API in this PR: https://github.com/patternfly/patternfly-react/pull/2290/files. Basically in the ChipGroup component you'd accept headingLevel as a prop and pass it to a ChipGroupContext.Provider, and then in ChipGroupToolbarItem you'd render a ChipGroupContext.Consumer that takes a function for a child, and this function can pull the headingLevel out of its arguments and return the children rendered with that headingLevel.

If you need any help, feel free to let me know!

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Jun 25, 2019

@mturley no problem I will be working on this.
Thanks for your feedback!

@boaz0 boaz0 force-pushed the boaz0:closes_2167 branch 2 times, most recently from 103b64a to 6eb1ffd Jun 25, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Jun 25, 2019

@mturley updated PR feel free to review and give some feedback.

Copy link
Collaborator

mturley left a comment

Thanks for your quick update @boaz0 ! Your actual changes look great, I'm just a little obsessed with code style today.

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
@boaz0 boaz0 force-pushed the boaz0:closes_2167 branch from 6eb1ffd to 65e2fde Jun 26, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Jun 26, 2019

@mturley thanks a lot! Would you mind reviewing this again?

@dlabaj
dlabaj approved these changes Jun 27, 2019
Copy link
Contributor

jessiehuff left a comment

LGTM! :)

@tlabaj
tlabaj approved these changes Jun 27, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

Copy link
Contributor

tlabaj left a comment

I have the same question as Joachim

@tlabaj
tlabaj approved these changes Jun 27, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit d87ce91 into patternfly:master Jun 27, 2019
8 checks passed
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
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Jun 28, 2019

@boaz0 boaz0 deleted the boaz0:closes_2167 branch Jun 28, 2019
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.