chore(Alert): updated toggle and close styling per audit#7086
chore(Alert): updated toggle and close styling per audit#7086mcoker merged 4 commits intopatternfly:mainfrom
Conversation
|
Preview: https://patternfly-pr-7086.surge.sh A11y report: https://patternfly-pr-7086-a11y.surge.sh |
I think the comment was just around the button not being the right height from the line-height you fixed, so I'd expect any add'l actions to be the same size as the close button is now. |
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
LGTM, but I'd like to use the semantic spacer for the toggle margin if possible. Left another comment, but I can add that as a follow-up if you don't want to add anything additional to this PR.
| @@ -226,10 +228,6 @@ | |||
| margin-block-end: var(--#{$alert}__action--MarginBlockEnd); | |||
| margin-inline-end: var(--#{$alert}__action--MarginInlineEnd); | |||
| transform: translateY(var(--#{$alert}__action--TranslateY)); | |||
There was a problem hiding this comment.
wdyt about removing this? It's causing the close button to be off-center. My guess is it had something to do with the button being smaller? From this PR, here's what it looks like with/without the transform
Before
After
And while I'm here, I don't know why we have display: flex and margin-block-start on .pf-v6-c-alert__icon. Taking both of those off aligns the icon as expected with the text because both the icon and text are inline with the same font-size and line-height.
That's scope creep though, so I can create a follow-up unless you want to give it a shot in this PR.
| --#{$alert}__toggle--MarginBlockEnd: calc(-1 * var(--pf-t--global--spacer--control--vertical--default)); | ||
| --#{$alert}__toggle--MarginInlineEnd: var(--pf-t--global--spacer--sm); | ||
| --#{$alert}__toggle--MarginInlineStart: calc(-1 * var(--pf-t--global--spacer--action--horizontal--plain--default)); | ||
| --#{$alert}__toggle--MarginInlineEnd: var(--pf-t--global--spacer--action--horizontal--plain--default); |
There was a problem hiding this comment.
I think this one should just use a regular spacer - or do you think it should use the action spacer? If so can you add why?
| --#{$alert}__toggle--MarginInlineEnd: var(--pf-t--global--spacer--action--horizontal--plain--default); | |
| --#{$alert}__toggle--MarginInlineEnd: var(--pf-t--global--spacer--sm); |
There was a problem hiding this comment.
Would we want this to also be the horizontal spacer for the same reason as using it for the MarginInlineStart var?
Actually wondering if it would make sense to reverse the tokens, so that the sm token is used in the calc for MarginInlineStart, and the horizontal-plain one is used for MarginInlineEnd. If the horizontal token were customized, would we want that to affect the InlineStart spacing? It could cause the toggle to look like it's "breaking out" of the alert container (below screenshot is using the horizontal plain token on InlineStart only with an override of 24px, using sm token on InlineEnd):
Versus switching the tokens:
Though maybe this is also a case for not using the horizontal token for either var.
There was a problem hiding this comment.
Discussed off GitHub. We're gonna go with reverting the MarginInlineEnd to the sm spacer, with the intent being that additional overrides may need to be taken (e.g. overriding the --#{$alert}--PaddingInlineStart var).
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Just one comment but otherwise LGTM! Here's the visual regression report from any example URL with "alert"
BackstopJS Report.pdf
BackstopJS Report.pdf
BackstopJS Report.pdf
thatblindgeye
force-pushed
the
alertAuditUpdates
branch
from
c67f4c5 to
9e98472
Compare
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
LPTM! Thanks @thatblindgeye! 🥳
andrew-ronaldson
left a comment
andrew-ronaldson
left a comment
There was a problem hiding this comment.
This if fan-freaking-tastic!
5 participants
Closes #7054
Toggle before update:
Toggle after update:
Close button before update:
Close button after update:
Do we ever intend for consumers to ever be able to have multiple actions in an Alert, not just a close button? Wondering if having the action button be larger would cause issues if that's the case.