feat(Chip): Add the Chip component to PF4 #1076

boaz0 · 2018-12-18T03:55:21Z

What: #976

patternfly-build · 2018-12-18T04:12:46Z

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

coveralls · 2018-12-18T04:13:13Z

Pull Request Test Coverage Report for Build 3877

13 of 13 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 81.278%

Totals
Change from base Build 3866:	0.07%
Covered Lines:	4381
Relevant Lines:	5087

💛 - Coveralls

patternfly-build · 2018-12-18T04:13:26Z

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

karelhala

Great implementation! I like how you split the component into multiple small components and kept them as simple as possible.

packages/patternfly-4/react-core/src/components/Chip/Chip.js

packages/patternfly-4/react-core/src/components/Chip/examples/SimpleChip.js

packages/patternfly-4/react-core/src/components/Chip/ChipWrapper.js

packages/patternfly-4/react-core/src/components/Chip/Chip.js

dlabaj

@boaz1337 Looks like there's some conflicts on this one that need to be resolved.

tlabaj

Looks good jus had a few questions (inline with the code).

packages/patternfly-4/react-core/src/components/Chip/Chip.js

mcarrano

Looks good @boaz0 . For Core had separate components for Chip and Chip Group. Will Chip Group be coming separately for React also?

When the text truncates, can we add a tooltip to display the full label?

mcoker

Looks good overall, but you don't need aria-label="4 more" on the .pf-m-overflow example. aria-label is only needed when an icon is used with no supporting text.

boaz0 · 2019-01-09T21:12:24Z

@tlabaj thanks. I removed ChipText and ChipWrapper as you suggested and used getUniqueId to set a default id.

@mcarrano yes. I will add ChipGroup later. I also did as you suggested: adding tooltip.

@mcoker thanks. I removed aria-label. Let me know if I'm missing anything.

Thank you all.

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>

tlabaj · 2019-01-10T15:10:46Z

packages/patternfly-4/react-core/src/components/Chip/Chip.d.ts

+};
+
+export interface ChipProps extends Omit<HTMLProps<HTMLDivElement>, 'children' > {
+  children: string;


This file should be updated as well. It is missing id and onClick and isOverflowed is no longer a prop.

Can you get rid of the Omit 'children' and remove children from the interface? Also don't need id and onClick I believe since they are included in HTMLProps

tlabaj · 2019-01-10T15:17:40Z

packages/patternfly-4/react-core/src/components/Chip/Chip.js

+};
+
+const Chip = ({ variant, onClick, children, id, position, className, ...props }) => {
+  const idChip = id || getUniqueId()


This will not work if there are multiple chips on the page. They will all have the same id. You can wrap it with the GenerateId component to get a unique id.

tlabaj · 2019-01-10T15:25:34Z

packages/patternfly-4/react-core/src/components/Chip/Chip.js

+    case ChipVariant.overflow:
+      return (
+        <div className={css(styles.chip, styles.modifiers.overflow, className)} {...props}>
+          <ChipButton onClick={onClick} aria-label="Expand chip">


aria-label should be removed here, to match core. It is not needed since the button has text.

tlabaj · 2019-01-10T17:58:03Z

packages/patternfly-4/react-core/src/components/Chip/Chip.js

+          <span className={css(styles.chipText)} id={idChip}>
+            {children}
+          </span>
+          <ChipButton onClick={onClick} aria-label="Remove" id={`remove_${idChip}`} aria-labelledby={`remove_${idChip} ${idChip}`}>


For localization purposes, all user visible/audible text should be customizable. Maybe we can add an optional prop for the button's aria Label.

jschuler · 2019-01-14T17:15:53Z

packages/patternfly-4/react-core/src/components/Chip/Chip.d.ts

+};
+
+export interface ChipProps extends Omit<HTMLProps<HTMLDivElement>, 'children' > {
+  children: string;


Can you get rid of the Omit 'children' and remove children from the interface? Also don't need id and onClick I believe since they are included in HTMLProps

jschuler · 2019-01-14T17:19:55Z

packages/patternfly-4/react-core/src/components/Chip/Chip.js

+          </ChipButton>
+        </div>
+      );
+      return children.length < 16 ? (


Curious why 16? Should this be customizable?

ibolton336 · 2019-01-14T18:03:03Z

@boaz0 I have opened a new PR for this as we need to expedite the process. I was unable to find a way to contact you but perhaps we can meet & pair program if your are in the Raleigh area this week. Thank you for your hard work on this. I have kept all of your commits intact & just cherry picked from your branch. New PR is #1180

boaz0 · 2019-01-15T09:17:02Z

@ibolton336 no problem.

Although, I am in Raleigh this week, I don't think I have the time to join in.

Thanks for the invitation, though. 😀

karelhala reviewed Dec 18, 2018

View reviewed changes

packages/patternfly-4/react-core/src/components/Chip/Chip.js Outdated Show resolved Hide resolved

tlabaj added the PF4 label Dec 18, 2018

ibolton336 suggested changes Dec 19, 2018

View reviewed changes

tlabaj added ux review labels Dec 19, 2018

tlabaj requested a review from christiemolloy December 19, 2018 20:53

ibolton336 previously approved these changes Dec 20, 2018

View reviewed changes

dlabaj previously approved these changes Jan 3, 2019

View reviewed changes

boaz0 dismissed stale reviews from dlabaj and ibolton336 via bb59c4d January 4, 2019 10:13

tlabaj reviewed Jan 8, 2019

View reviewed changes

packages/patternfly-4/react-core/src/components/Chip/Chip.js Outdated Show resolved Hide resolved

packages/patternfly-4/react-core/src/components/Chip/Chip.js Outdated Show resolved Hide resolved

tlabaj requested review from mcarrano and mcoker January 8, 2019 20:49

mcarrano reviewed Jan 8, 2019

View reviewed changes

mcoker reviewed Jan 8, 2019

View reviewed changes

feat(Chip): Add the Chip component to PF4

aa1d3fc

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>

tlabaj requested changes Jan 10, 2019

View reviewed changes

tlabaj added the enhancement 🚀 label Jan 11, 2019

jschuler reviewed Jan 14, 2019

View reviewed changes

ibolton336 closed this Jan 15, 2019

boaz0 deleted the chips branch January 15, 2019 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Chip): Add the Chip component to PF4 #1076

feat(Chip): Add the Chip component to PF4 #1076

boaz0 commented Dec 18, 2018 •

edited by tlabaj

Loading

patternfly-build commented Dec 18, 2018

coveralls commented Dec 18, 2018 •

edited

Loading

patternfly-build commented Dec 18, 2018

karelhala left a comment

dlabaj left a comment

tlabaj left a comment

mcarrano left a comment

mcoker left a comment

boaz0 commented Jan 9, 2019

tlabaj Jan 10, 2019

jschuler Jan 14, 2019

tlabaj Jan 10, 2019

tlabaj Jan 10, 2019

tlabaj Jan 10, 2019

jschuler Jan 14, 2019

jschuler Jan 14, 2019

ibolton336 commented Jan 14, 2019 •

edited

Loading

boaz0 commented Jan 15, 2019

feat(Chip): Add the Chip component to PF4 #1076

feat(Chip): Add the Chip component to PF4 #1076

Conversation

boaz0 commented Dec 18, 2018 • edited by tlabaj Loading

patternfly-build commented Dec 18, 2018

coveralls commented Dec 18, 2018 • edited Loading

Pull Request Test Coverage Report for Build 3877

💛 - Coveralls

patternfly-build commented Dec 18, 2018

karelhala left a comment

Choose a reason for hiding this comment

dlabaj left a comment

Choose a reason for hiding this comment

tlabaj left a comment

Choose a reason for hiding this comment

mcarrano left a comment

Choose a reason for hiding this comment

mcoker left a comment

Choose a reason for hiding this comment

boaz0 commented Jan 9, 2019

tlabaj Jan 10, 2019

Choose a reason for hiding this comment

jschuler Jan 14, 2019

Choose a reason for hiding this comment

tlabaj Jan 10, 2019

Choose a reason for hiding this comment

tlabaj Jan 10, 2019

Choose a reason for hiding this comment

tlabaj Jan 10, 2019

Choose a reason for hiding this comment

jschuler Jan 14, 2019

Choose a reason for hiding this comment

jschuler Jan 14, 2019

Choose a reason for hiding this comment

ibolton336 commented Jan 14, 2019 • edited Loading

boaz0 commented Jan 15, 2019

boaz0 commented Dec 18, 2018 •

edited by tlabaj

Loading

coveralls commented Dec 18, 2018 •

edited

Loading

ibolton336 commented Jan 14, 2019 •

edited

Loading