fix(label): add status variants#6227
Conversation
|
Preview: https://patternfly-pr-6227.surge.sh A11y report: https://patternfly-pr-6227-a11y.surge.sh |
mcoker
requested review from
andrew-ronaldson,
lboehling,
mattnolting and
thatblindgeye
andrew-ronaldson
left a comment
andrew-ronaldson
left a comment
There was a problem hiding this comment.
Apologies if I missed a conversation. Should the status lables have the status color on the icon or is it just inheriting the colour from the text? In Figma the icons all have the status color.
From a design guidelines perspective I think we should recommend using a status label with the icons so they stand out from the non status versions. @lboehling do you have any thoughts here?
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Unless it'd be better as a followup, can we update the status labels to use appropriate icons? Right now they're all using the info icon, when success should have a checkmark, warning the triangle !, etc.
|
|
||
| {{> label | ||
| label--id=(concat label--variants--id '-icon') | ||
| label-icon--value="info-circle" |
There was a problem hiding this comment.
Just a nit: since we're showing status labels that would use this icon, we might want to avoid using a "statusy" icon outside a status context. WdYT?
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
✅ As a followup we could add screen reader text to the status labels, but it might be worth evaluating where else we may want to do that in other components and/or if it makes sense to always render it vs rendering it only if the visible text doesn't convey that status well.
andrew-ronaldson
left a comment
andrew-ronaldson
left a comment
There was a problem hiding this comment.
Rockstar. LGTM
srambach
left a comment
srambach
left a comment
There was a problem hiding this comment.
LGTM. Kind of a bummer to have to require .pf-m-filled but makes sense. 🏷️
lboehling
left a comment
lboehling
left a comment
There was a problem hiding this comment.
looks good! I am noticing that the filled background color for the status labels are referencing a border color token instead of the global/color/status/[success/danger/warning/etc.] tokens
|
@lboehling great catch!! Looks like all of the filled background color tokens were wrong 🤦♂️ Updated, you can see the tokens changed here - ea51c90 FWIW I just did a search/replace for |
|
perfect! thanks @mcoker!! |
|
🎉 This PR is included in version 6.0.0-alpha.69 🎉 The release is available on: Your semantic-release bot 📦🚀 |
6 participants
fixes #6226
label--isRemovabletolabel--IsRemovableidto the label text element when a label is removable, since the close button currently references a non-existentidinaria-labelledbyvia something like<button id="close-btn" arial-label="Remove" aria-labelledby="close-btn label-text">1/23/24
.pf-m-filledfor the filled styling, which gives us a modifier for filled-specific styling'..pf-m-filledor.pf-m-outlinefor the variant on all labels except for the add and overflow labels as those are structured a bit differently currently.