-
Notifications
You must be signed in to change notification settings - Fork 355
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(Chip): use Label internally, deprecate Chip #10049
Conversation
Preview: https://patternfly-react-pr-10049.surge.sh A11y report: https://patternfly-react-pr-10049-a11y.surge.sh |
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 |
I'll revert the code-editor and try it out again. Update: @nicolethoen I did just omit the |
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. |
There was a problem hiding this comment.
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.
packages/react-core/src/deprecated/components/Chip/__tests__/Chip.test.tsx
Outdated
Show resolved
Hide resolved
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? 🙈 |
{currentChips.map((currentChip) => ( | ||
<Chip key={currentChip} onClick={() => deleteChip(currentChip)}> | ||
<Label key={currentChip} variant="outline" onClose={() => deleteChip(currentChip)}> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
@@ -1,8 +1,8 @@ | |||
import * as React from 'react'; | |||
import * as ReactDOM from 'react-dom'; | |||
import { ToolbarItem, ToolbarItemProps } from './ToolbarItem'; | |||
import { ChipGroup } from '../Chip'; | |||
import { Chip } from '../Chip'; | |||
import { ChipGroup } from '../../deprecated/components/Chip'; |
There was a problem hiding this comment.
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?
{currentChips.map((currentChip) => ( | ||
<Chip key={currentChip} onClick={() => deleteChip(currentChip)}> | ||
<Label key={currentChip} variant="outline" onClose={() => deleteChip(currentChip)}> |
There was a problem hiding this comment.
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.
@@ -40,9 +40,9 @@ export const CodeEditorShortcutMainHeader: React.FunctionComponent = () => { | |||
<GridItem style={{ textAlign: 'right', marginRight: '1em' }}> | |||
{shortcut.keys | |||
.map((key) => ( | |||
<Chip key={key} isReadOnly> | |||
<Label variant="outline" key={key}> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved!
There was a problem hiding this comment.
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:
Thanks for your contribution! 🎉 |
What: Closes #9983