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

fix(chipGroup): Allow variable number of chips to be displayed #2878

Merged
merged 6 commits into from Sep 16, 2019

Conversation

@jessiehuff
Copy link
Contributor

jessiehuff commented Sep 6, 2019

Fixes #1855

Jessie added 3 commits Jul 10, 2019
Jessie
Jessie
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 6, 2019

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

Jessie added 3 commits Sep 6, 2019
Jessie
Jessie
Jessie
@kmcfaul
kmcfaul approved these changes Sep 9, 2019
Copy link
Contributor

kmcfaul left a comment

Looks good!

@tlabaj tlabaj added the ux review label Sep 9, 2019
@tlabaj tlabaj requested a review from mcarrano Sep 9, 2019
@tlabaj tlabaj self-assigned this Sep 9, 2019
Copy link
Member

mcarrano left a comment

Does this setting apply to the ChipGroup Toolbar, ChipGroup Multi-Select, or both?

@jessiehuff

This comment has been minimized.

Copy link
Contributor Author

jessiehuff commented Sep 10, 2019

@mcarrano the prop is on the ChipGroup component so it should work for both ChipGroup Toolbar and ChipGroup Multi-Select. :)

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Sep 11, 2019

@jessiehuff I guess that the reason I asked this question was that the default for setting how many chips to show before overflow is 3, however in the ChipGroup Toolbar example you are showing 4 chips without overflow. I would have expected to see the overflow on the fourth chip unless you set the overflow to greater. Am I understanding this correctly?

@jessiehuff

This comment has been minimized.

Copy link
Contributor Author

jessiehuff commented Sep 12, 2019

@mcarrano Since the prop is on the ChipGroup component itself, it sets the children of ChipGroup to the default numChips. So in the case of ChipGroup Toolbar, it's showing 3 ChipGroupToolbarItems, not necessarily the chips inside each ChipGroupToolbarItem.

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

tlabaj left a comment

LGTM

@@ -21,6 +21,8 @@ export interface ChipGroupProps extends React.HTMLProps<HTMLDivElement> {
withToolbar?: boolean;
/** Set heading level to the chip item label */
headingLevel?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6';
/** Set number of chips to show before overflow */

This comment has been minimized.

Copy link
@tlabaj

tlabaj Sep 12, 2019

Contributor

Maybe change this to say Set number of items to show before overflow since or number of chips\chip groups to show before overflow

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Sep 12, 2019

@jessiehuff @tlabaj it sounds like there is a misunderstanding about what is intended. I see this as 3 separate chip groups, not as one chip group with 3 items. I don't think we want to hide an entire category of chips. It's also the case that we would not want to show a large number of chips in a grouping even if there were less than 3 groups. Will be glad to talk this through live if that helps to clarify the intended behavior. This document talks a bit more about how I am seeing the ChipGroup from a design perspective.

Copy link
Member

mcarrano left a comment

After further conversations, I think that this PR satisfies it's aims within the context of the current implementation of ChipGroups. There is currently an issue open on Core to refactor implementation of Chips Groups to be more consistent with design intent as described here: patternfly/patternfly-next#1837

@mcarrano mcarrano added ux approved and removed ux review labels Sep 16, 2019
@tlabaj tlabaj merged commit 6bbff0c into patternfly:master Sep 16, 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
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 16, 2019

Your changes have been released in:

  • @patternfly/react-core@3.104.2
  • @patternfly/react-docs@4.13.4
  • @patternfly/react-inline-edit-extension@2.11.38
  • demo-app-ts@3.0.1
  • @patternfly/react-table@2.20.18
  • @patternfly/react-topology@2.8.36
  • @patternfly/react-virtualized-extension@1.2.26

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.