feat(Chip): use Label internally, deprecate Chip by kmcfaul · Pull Request #10049

feat(Chip): use Label internally, deprecate Chip by kmcfaul · Pull Request #10049 · patternfly/patternfly-react

Styling on the chips will look a little off (especially around the close buttons) until #10037 is merged and pulled in.

@nicolethoen Had some issues with chips in examples for react-core/react-code-editor not being found with the deprecated import path. I ended up updating those to directly use Label/LabelGroup since that will be the intended pattern in the future, though I could back that out and still use Chip if we want. I think I was just missing an import in the md file in retrospect.

@kmcfaul the only thing that causes concern about using Label in the code editor package is that code editor is the test case for someone external to the react-core package importing the deprecated component. It should be possible...

kmcfaul linked an issue

Jan 30, 2024

that may be closed by this pull request

I'll revert the code-editor and try it out again.

Update: @nicolethoen I did just omit the md import on accident the first time around, importing the deprecated Chip into code-editor (and anywhere else) should work fine. Should I revert the example updates as well or leave them using Label? Should I re-update CodeEditor?

Looks like CodeEditor is still odd

Do I remember right that we had to alias imports from the deprecated/next directories before to get things to work properly?

Hm that looks in line with how Chip is showing up currently in the rest of the PR.

The close buttons on the Chips (Labels) will be too large and make the chip itself larger until the Label PR goes in.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple comments below. It'd also be worth having a codemod at least warn consumers that Chip is now using our Label component internally/that there may be markup changes (like Chip now no longer using aria-labelledby). This would probably have to be separate from the codemod that updates theimport path of Chip.

okay... so now that I know it works with the deprecated path - i'm (ashamedly) feeling like we likely don't want to be using the deprecated component in our actual examples, right? so we probably do want it updated to use the label 😬

Does that make sense? 🙈

		<LabelGroup>
		{currentChips.map((currentChip) => (
		<Chip key={currentChip} onClick={() => deleteChip(currentChip)}>
		<Label key={currentChip} variant="outline" onClose={() => deleteChip(currentChip)}>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we adding outline variant here? I ask because we may need a code mode, or at least documentation stating when we would want to do that?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because internally, Chips are Labels with the outline variant. We do have a codemod issue opened to cover converting the deprecated Chip to Label (though I believe the goal to at minimum update the Chip to the deprecated path which is covered by a different codemod).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need to update the path and also add the variant. Or at least a warning.

		import { ToolbarItem, ToolbarItemProps } from './ToolbarItem';
		import { ChipGroup } from '../Chip';
		import { Chip } from '../Chip';
		import { ChipGroup } from '../../deprecated/components/Chip';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the Label instead?

		<LabelGroup>
		{currentChips.map((currentChip) => (
		<Chip key={currentChip} onClick={() => deleteChip(currentChip)}>
		<Label key={currentChip} variant="outline" onClose={() => deleteChip(currentChip)}>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about the variant here.

		{shortcut.keys
		.map((key) => (
		<Chip key={key} isReadOnly>
		<Label variant="outline" key={key}>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will also nee guidance/ code mode for isReadOnly.
And I have the same question here about the outlined variant.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think converting isReadOnly should be easy enough to cover in the same codemod issue to convert Chip to Label, as it effectively means that Label shouldn't have any onClick handler.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Your changes have been released in:

@patternfly/react-code-editor@6.0.0-alpha.34
@patternfly/react-core@6.0.0-alpha.34
@patternfly/react-docs@7.0.0-alpha.35
@patternfly/react-drag-drop@6.0.0-alpha.15
@patternfly/react-integration@6.0.0-alpha.17
demo-app-ts@5.1.1-alpha.33
@patternfly/react-table@6.0.0-alpha.34

Thanks for your contribution! 🎉