fix(vertical tabs): update with unified tokens#99
Conversation
…te tab border radius, spacing, color, and hover background color
|
(fixed) I'm looking into the jest failures. |
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Just a couple of comments, otherwise looks great!
|
|
||
| > a { | ||
| color: var( --pf-t--global--text--color--regular); | ||
| color: var( --pf-t--global--text--color--subtle); |
There was a problem hiding this comment.
Just want to make sure this was confirmed in design? It matches figma, but wasn't called out as a change to make. Also figma uses the default text color for the active/selected tab state, so if we would want to confirm that if we make this change, too.
There was a problem hiding this comment.
Got more info from Design on this and updated for default to be subtle and hover/active to be default.
| text-decoration: none; | ||
| display: block; | ||
| margin-inline-start: var(--pf-t--global--spacer--md); | ||
| margin-inline-start: calc(var(--pf-t--global--spacer--md) - 3px); |
There was a problem hiding this comment.
Want to reference the var here in case the value for that border width token ever changes.
| margin-inline-start: calc(var(--pf-t--global--spacer--md) - 3px); | |
| margin-inline-start: calc(var(--pf-t--global--spacer--md) - var(--pf-t--global--border--width--extra-strong)); |
| @@ -18,15 +18,30 @@ | |||
| margin-inline-start: var(--pf-t--global--spacer--md); | |||
There was a problem hiding this comment.
Looking at the figma file, the gap between the accent color and the nav item is smaller than it appears in the preview. I think this is supposed to be a small spacer.
There was a problem hiding this comment.
@andrew-ronaldson, I can see the difference Nicole is pointing out but could you clarify? In the dev notes I'm seeing a calc(spacer-md - 3px) (3px currently = --pf-t--global--border--width--extra-strong) between the indicator and where the padding starts for the a tag. Then the padding around the text for the a tag is set to top/bottom spacer-xs and left/right spacer md.
There was a problem hiding this comment.
The way you have the spacing aligns with the visual presentation of vertical Tabs component just implemented differently. Dropping down to a small makes a 5px gap and that feels too narrow. We could change the accent to be outset of the container but then it has a negative margin and messes with left alignment. I think this is fine as you have it and we can tinker more as needed.
6 participants
closes: #96
version bumps on PatterFly deps to v6.5.x
update to unified tokens for vertical tab styles
update to scss @import to @use