feat(DescriptionList): apply tokens#6129
Conversation
|
Preview: https://patternfly-pr-6129.surge.sh A11y report: https://patternfly-pr-6129-a11y.surge.sh |
There was a problem hiding this comment.
Noticing a couple of things:
- This is pre-existing so we can do a followup, but the "Help text" example should have teh
pf-m-help-textclass applied to the.pf-v5-c-description-list__term > .pf-v5-c-description-list__textelement to apply the underlined styling - Figma has different font size styling for card variants than shown here. There's a "small" variant that looks to use the font sizes of a default description list, a "default" variant that has slightly larger font sizing, and a "large" variant. The
__termelement also has its font size updated (right now we're only updating the__descriptionfont sizing). - The text blends in with the card variant in dark mode, but I imagine that would be resolve when the card component gets Penta-fied.
| // Display modifiers | ||
| --#{$description-list}--m-display-lg__description--FontSize: var(--#{$pf-global}--FontSize--lg); | ||
| --#{$description-list}--m-display-2xl__description--FontSize: var(--#{$pf-global}--FontSize--2xl); | ||
| --#{$description-list}--m-display-lg__description--FontSize: var(--pf-t--global--font--size--body--lg); // TODO replace with new font tokens when available |
There was a problem hiding this comment.
Looks like this is setting the font size to just 16px, Figma show it shouls be 22px for the description text for large styling.
| --#{$description-list}--m-display-lg__description--FontSize: var(--#{$pf-global}--FontSize--lg); | ||
| --#{$description-list}--m-display-2xl__description--FontSize: var(--#{$pf-global}--FontSize--2xl); | ||
| --#{$description-list}--m-display-lg__description--FontSize: var(--pf-t--global--font--size--body--lg); // TODO replace with new font tokens when available | ||
| --#{$description-list}--m-display-2xl__description--FontSize: var(--pf-t--global--font--size--heading--lg); // TODO replace with new font tokens when available |
There was a problem hiding this comment.
Figma only has sizing for small, default, and large for the card variant of the component. Do we still need this var and the associated modifier class?
There was a problem hiding this comment.
L🎄TM! Left a couple of comments we can follow up on.
| // help text | ||
| --#{$description-list}__text--m-help-text--TextDecorationColor: var(--#{$pf-global}--BorderColor--200); | ||
| --#{$description-list}__text--m-help-text--TextDecorationThickness: var(--#{$pf-global}--BorderWidth--sm); | ||
| --#{$description-list}__text--m-help-text--TextDecorationColor: var(--pf-t--global--icon--color--regular); |
There was a problem hiding this comment.
This matches figma, but @lboehling says that's not accurate and we should just use whatever the text color is here. In another example, I just used currentcolor here, which will tell it to inherit whatever the text color is.
| --#{$description-list}__text--m-help-text--TextDecorationColor: var(--pf-t--global--icon--color--regular); | |
| --#{$description-list}__text--m-help-text--TextDecorationColor: currentcolor; |
| --#{$description-list}__text--m-help-text--TextDecorationColor: var(--#{$pf-global}--BorderColor--200); | ||
| --#{$description-list}__text--m-help-text--TextDecorationThickness: var(--#{$pf-global}--BorderWidth--sm); | ||
| --#{$description-list}__text--m-help-text--TextDecorationColor: var(--pf-t--global--icon--color--regular); | ||
| --#{$description-list}__text--m-help-text--TextDecorationThickness: var(--pf-t--global--border--width--100); |
There was a problem hiding this comment.
Just a note here, but we'll want to update this to use a semantic token
|
🎉 This PR is included in version 6.0.0-alpha.57 🎉 The release is available on: Your semantic-release bot 📦🚀 |
closes #6101
with new v6 branch. Old PR: #6102