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

fix(chipGroup): Allow variable number of chips to be displayed by jessiehuff · Pull Request #2878 · patternfly/patternfly-react · GitHub

/ patternfly-react Public

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

Merged
tlabaj merged 6 commits into
patternfly:masterfrom
jessiehuff:fix/chipgroupShow
Sep 16, 2019
ConversationCommits (6)ChecksFiles changed
Merged

fix(chipGroup): Allow variable number of chips to be displayed#2878
tlabaj merged 6 commits into
patternfly:masterfrom
jessiehuff:fix/chipgroupShow

Conversation

jessiehuff commented Sep 6, 2019

Copy link
Copy Markdown
Contributor

Fixes #1855

patternfly-build commented Sep 6, 2019

Copy link
Copy Markdown
Collaborator

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

kmcfaul approved these changes Sep 9, 2019
View reviewed changes

kmcfaul 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 a review from mcarrano September 9, 2019 19:23
tlabaj self-assigned this Sep 9, 2019
mcarrano reviewed Sep 9, 2019
View reviewed changes

mcarrano 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

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

jessiehuff commented Sep 10, 2019

Copy link
Copy Markdown
Contributor Author

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

mcarrano commented Sep 11, 2019

Copy link
Copy Markdown
Member

@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 commented Sep 12, 2019

Copy link
Copy Markdown
Contributor Author

@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 approved these changes Sep 12, 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 reviewed Sep 12, 2019
View reviewed changes
Comment thread packages/patternfly-4/react-core/src/components/ChipGroup/ChipGroup.tsx
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 */

tlabaj Sep 12, 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 change this to say Set number of items to show before overflow since or number of chips\chip groups to show before overflow

mcarrano commented Sep 12, 2019

Copy link
Copy Markdown
Member

@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.

mcarrano approved these changes Sep 16, 2019
View reviewed changes

mcarrano 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

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#1837

tlabaj merged commit 6bbff0c into patternfly:master Sep 16, 2019

patternfly-build commented Sep 16, 2019

Copy link
Copy Markdown
Collaborator

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! 🎉

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

kmcfaul kmcfaul approved these changes
+2 more reviewers
tlabaj tlabaj approved these changes
mcarrano mcarrano 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.

ChipGroup should allow a variable number of chips to be displayed

5 participants

Back | FazBrowse Home | New Git URL