feat(Button): add high contrast to plain/link hover+clicked - #7734
Conversation
|
Preview: https://pf-pr-7734.surge.sh A11y report: https://pf-pr-7734-a11y.surge.sh |
|
Should the other variants of button have a thicker border for the clicked state? I'm noticing that the border becomes thicker for the clicked state in the figma, but this pattern doesn't apply to the other variants which already had a border (hover = clicked border width). |
|
The figma file for button is also a little confusing on the CTA button. I've applied the styling to the link CTA but its design is more squared than the rounded like the Figma shows. |
e91e43f to
2f22545
Compare
|
@lboehling looks like the update to the plain/no-padding button may have caused an issue with the label close button. That button has custom padding already, but it pushes the border outside of the label. If I get rid of the custom padding and just use the new fake padding we added to plain/no-padding, it goes from this: 
to this 
WDYT? Also I looked up everywhere we use the plain/no-padding button in our components and those spots look ok, you can verify in this PR. For reference, they're:
|
|
@kmcfaul in chatting with @lboehling, we decided to remove the border from the button's inline links. The main issue there is that those present like normal links - so if we draw a border around them, it follows that we would draw a border around regular links, too. However, regular links (and the button's "span" inline link) can wrap to multiple lines, then the border breaks. There is already enough contrast and distinction with links that they don't need the border, so we can get rid of the border we added to inline links. |
+1 to getting rid of the custom padding. i think that your update looks good. When i went to toggle off the custom padding, it did start to look like the "x" close icon was butting up too closely with the label text. Is there anyway to keep a little gap between the text and the label, and use the updated no-padding padding? You can see the difference below (First 2 with inline start custom padding turned off, Second 2 with it left on). note - im not suggesting we keep the inline custom padding for the close action since it off centers the border, but if there was a way to add in that spacing between the close action and text again that would be 💯. 
|
|
Chatted about the label close spacing and decided to 1) remove the custom padding as design suggested, and 2) add back the space that was lost from the padding being removed on the left and right sides of the icon. Just glancing at it, I think that update would look like:
|
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Just a couple of small changes. And chatted with design about the border overflowing compact labels and we landed on using a custom inset for plain/no-padding actions in compact labels by removing the top and bottom inset.
You could do it a bunch of ways, but I would probably(?) put it in this spot, you could do something like this. Update from:
.#{$button} {
--#{$button}--FontSize: var(--#{$label}__actions--c-button--FontSize);
to
.#{$button} {
@at-root .#{$label}.pf-m-compact & {
--#{$button}--m-plain--m-no-padding--after--Inset: 0 calc(#{pf-size-prem(2px) * -1)
}
--#{$button}--FontSize: var(--#{$label}__actions--c-button--FontSize);
That should compile to
.pf-v6-c-label.pf-m-compact .pf-v6-c-label__actions .pf-v6-c-button {
--pf-v6-c-button--m-plain--m-no-padding--after--Inset: 0 -2px;
}
srambach
left a comment
srambach
left a comment
There was a problem hiding this comment.
Just curious if we need the link and plain clicked and hover border color variables? Are they here for completeness?
|
@lboehling can you take a look at this PR? Specifically it changes:
|
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Looks awesome to me! Just one last request on the var names for the insets, we typically use the word "offset" when we move a border/outline from some relative position, and we can replace after with border which is easier to read and lets us update the structure down the road (border could go on the ::before element) and the var name shouldn't need to be updated.
5 participants
Closes #7591
Closes #7614
Closes #7742