fix(alert): update title tokens#6775
Conversation
|
Preview: https://patternfly-pr-6775.surge.sh A11y report: https://patternfly-pr-6775-a11y.surge.sh |
Includes: notification drawer, card, hint and popover.
|
lookin gooood but i'm noticing the icons are little out of alignment |
Yeah - there's a little adjustment on the icon that we can change. Line 32 |
|
On popover, the close button overlaps the body text a little now and the baseline of the text isn't aligned with the close button. @mcoker do you think we need a header wrapper? 2024-06-11_16-34-44.mp4 |
|
@andrew-ronaldson @lboehling looks like the popover close button is misaligned from this style failing from We can resolve it by changing the That doesn't have to happen in this PR though, if we want to get this through and follow up on that button - up to you @andrew-ronaldson As an aside, a couple of questions from looking at this:
|
There was a problem hiding this comment.
Left some feedback for your perusal good sir 🥸
| --#{$alert}__title--FontFamily: var(--pf-t--global--font--family--body); | ||
| --#{$alert}__title--FontSize: var(--pf-t--global--font--size--body--default); |
There was a problem hiding this comment.
It's fine to have these as component vars, especially if we want to open the font-family and font-size to be themed, but technically we don't need them, since they should inherit both of these styles from the global styles. In particular, I'd say we drop --#{$alert}__title--FontFamily unless you want users to customize it. We only set the font-family to --pf-t--global--font--size--body--default in 2 other places:
- Multiple file upload "separator" text (the word "or" here - https://patternfly-pr-6775.surge.sh/components/file-upload/multiple-file-upload#basic)
- That's because the word "or" is part of the title, and the title uses the
--headingfont-family, so we need to reset it for the "or" text.
- That's because the word "or" is part of the title, and the title uses the
- Popover title text, which I'd advocate removing unless we expect the font-family to be themed. My guess as to why it's set is similar to what I'm assuming the change in this PR is. There was previously a declaration for
font-familyto set it to the--headingfont (for v5 or pre-now), so we updated the existing token to use--body, when it could just be removed entirely since the font-family for everything will be--bodyby default from PF's global styles
| --#{$alert}__title--FontFamily: var(--pf-t--global--font--family--body); | ||
| --#{$alert}__title--FontSize: var(--pf-t--global--font--size--body--default); | ||
| --#{$alert}__title--FontWeight: var(--pf-t--global--font--weight--body--bold); | ||
| --#{$alert}__title--LineHeight: var(--pf-t--global--font--line-height--heading); |
There was a problem hiding this comment.
We should remove this line-height--heading var if we don't expect users to theme the font-family. It was only there before because we always declare line-height use line-height--heading any time we use the font--family--heading token. PF global styles set line-height--body on everything by default.
| --#{$hint}__title--FontFamily: var(--pf-t--global--font--family--body); | ||
| --#{$hint}__title--FontSize: var(--pf-t--global--font--size--body--default); | ||
| --#{$hint}__title--FontWeight: var(--pf-t--global--font--weight--body--bold); | ||
| --#{$hint}__title--LineHeight: var(--pf-t--global--font--line-height--heading); |
There was a problem hiding this comment.
Same here about removing font-family and line-height unless we expect users to theme the font-family for hint titles. We can also technically remove --#{$hint}__title--FontSize: var(--pf-t--global--font--size--body--default); since the component will inherit the default body font-size from our global styles, though it's totally fine to leave it if we expect users to theme the font-size, which seems totally fair.
| --#{$notification-drawer}__list-item-header-title--FontFamily: var(--pf-t--global--font--family--body); | ||
| --#{$notification-drawer}__list-item-header-title--FontSize: var(--pf-t--global--font--size--body--default); | ||
| --#{$notification-drawer}__list-item-header-title--FontWeight: var(--pf-t--global--font--weight--body--bold); | ||
| --#{$notification-drawer}__list-item-header-title--LineHeight: var(--pf-t--global--font--line-height--heading); |
There was a problem hiding this comment.
Same comment here as in alert, hint, and popover.
| --#{$popover}__title-text--FontFamily: var(--pf-t--global--font--family--body); | ||
| --#{$popover}__title-text--FontSize: var(--pf-t--global--font--size--body--lg); | ||
| --#{$popover}__title-text--FontSize: var(--pf-t--global--font--size--body--default); |
There was a problem hiding this comment.
Same here as alert, hint, and notification drawer.
Local isn't working so pushing for the surge demo
removed font-family, size and line-height since they match the globals
updated the popover close button and removed conflicts with global variables.
| // Icon | ||
| --#{$alert}__icon--Color: var(--pf-t--global--icon--color--regular); | ||
| --#{$alert}__icon--MarginBlockStart: #{pf-size-prem(4px)}; | ||
| --#{$alert}__icon--MarginBlockStart: #{pf-size-prem(2px)}; |
There was a problem hiding this comment.
I'm not sure what else changed, but 4 looks right to me now. 🙈
| --#{$notification-drawer}__list-item-header-title--FontFamily: var(--pf-t--global--font--family--heading); | ||
| --#{$notification-drawer}__list-item-header-title--FontSize: var(--pf-t--global--font--size--heading--xs); | ||
| --#{$notification-drawer}__list-item-header-title--FontWeight: var(--pf-t--global--font--weight--heading--bold); | ||
| --#{$notification-drawer}__list-item-header-title--LineHeight: var(--pf-t--global--font--line-height--heading); |
There was a problem hiding this comment.
looks like we removed the __list-item-header-title (title in an individual notification item) font vars, then updated properties for __header-title. Was that intentional, or did you mean to make property updates to this block?
| --#{$popover}__title-text--FontFamily: var(--pf-t--global--font--family--body); | ||
| --#{$popover}__title-text--FontSize: var(--pf-t--global--font--size--body--lg); | ||
| --#{$popover}__title-text--FontWeight: var(--pf-t--global--font--weight--body--bold); | ||
| --#{$popover}__title-text--FontSize: var(--pf-t--global--font--size--heading--xs); |
There was a problem hiding this comment.
Since this is using a body font, I'd think we wouldn't want to use the heading font-size? Technically it should work just fine, but AFAIK we aren't mixing the body and heading tokens. The equivalent in the body font-size tokens would be --pf-t--global--font--size--body--lg.
thanks @mcoker Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com>
| --#{$popover}__title-icon--MarginInlineEnd: var(--pf-t--global--spacer--sm); | ||
| --#{$popover}__title-icon--Color: var(--pf-t--global--icon--color--regular); | ||
| --#{$popover}__title-icon--FontSize: var(--pf-t--global--font--size--heading--xs); | ||
| --#{$popover}__title-icon--FontSize: var(--pf-t--global--font--size--body--default); |
There was a problem hiding this comment.
| --#{$popover}__title-icon--FontSize: var(--pf-t--global--font--size--body--default); | |
| --#{$popover}__title-icon--FontSize: var(--pf-t--global--icon--size--font--body--default); |
| --#{$notification-drawer}__list-item-header-title--FontFamily: var(--pf-t--global--font--family--heading); | ||
| --#{$notification-drawer}__list-item-header-title--FontSize: var(--pf-t--global--font--size--heading--xs); | ||
| --#{$notification-drawer}__list-item-header-title--FontWeight: var(--pf-t--global--font--weight--heading--bold); | ||
| --#{$notification-drawer}__list-item-header-title--LineHeight: var(--pf-t--global--font--line-height--heading); |
There was a problem hiding this comment.
Looks like we're still using this --LineHeight here, which we can remove
|
pretty pretty!!! |
|
🎉 This PR is included in version 6.0.0-alpha.171 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fixes #6754