feat(Label): refactor for updated design and features #4165
Conversation
|
PF4 preview: https://patternfly-react-pr-4165.surge.sh |
| import { InfoCircleIcon } from '@patternfly/react-icons'; | ||
|
|
||
| SimpleLabel = () => ( | ||
| FilledLabels = () => ( |
There was a problem hiding this comment.
Rename to OutlinedLabels
| Blue removeable | ||
| </Label> <Label variant="outline" color="blue" icon={<InfoCircleIcon />} onClose={Function.prototype}> | ||
| Blue icon removeable | ||
| </Label> <Label variant="outline" color="blue" href="#"> |
There was a problem hiding this comment.
Would it make sense to set the hrefs to #filled or #outline for the link labels in both examples? At the moment when I click an outlined label with a link, it takes me back up to the top of the page.
redallen
left a comment
redallen
left a comment
There was a problem hiding this comment.
Slight API change, DRY tests, and this looks good!
| /** Aria label of the close button. */ | ||
| closeBtnAriaLabel?: string; | ||
| /** Id of the close button. */ | ||
| closeBtnId?: string; |
There was a problem hiding this comment.
I already know someone is going to want to customize the button they put in here... Can we just make the close button a React.ReactNode that the consumer can pass in, but has a default of:
<Button
type="button"
variant="plain"
onClick={onClose}
aria-label={'some-default'}
id={'some-default'}
aria-labelledby={`'some-default'`}
>
<TimesIcon />
</Button>if they pass onClose? It also cleans up having to support the closeBtnAriaLabel, closeBtnId, and closeBtnTextId props.
There was a problem hiding this comment.
I'll add a node property for a full custom button, and will condense the various default button props into a general 'closeBtnProps' object. That way users can pass in whatever labelling/id they want without needing multiple props or to define a full button.
packages/react-core/src/components/Label/__tests__/Label.test.tsx
Outdated
Show resolved
Hide resolved
|
CI is sad: |
|
I didn't change the documentation, so it has to be the change to the default button. Does it need an aria label maybe? |
|
@kmcfaul Yes, I think you need to set some default |
| onClose, | ||
| closeBtn, | ||
| closeBtnProps = { | ||
| 'aria-label': 'label-close-button' |
There was a problem hiding this comment.
Try:
closeBtnProps = {
'aria-label': 'label-close-button',
...closeBtnProps
}
or else when a consumer specifies just one property other than aria-label the aria-label won't be set. If that syntax doesn't work, on line 56 {...{'aria-label': 'label-close-button', ...closeBtnProps}} should.
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
6 participants
What: Closes #3688
The
onCloseprop, if present, will add the close button to the label, and thehrefprop, if present, will change the to an element.Breaking changes