-
Notifications
You must be signed in to change notification settings - Fork 977
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding tab borders #6593
base: current
Are you sure you want to change the base?
Adding tab borders #6593
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me @matthewshaver !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on Matt. Left some comments with some recommended adjustments. To try and summarize them i'll list them below:
- Swapping hard-coded hex values for available color variables at the top of this file
- Adjusting tab text color on hover
- Adjusting tab border color on hover
- Adjusting section border color in light mode
Let me know if you have any questions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than using hard-coded hex values, can we choose from the existing variables available at the top of this custom.css
file? That will help us stay within our brand colors, while also making it easier to update colors across the board if needed down the road.
/* Default color and border for tabs */ | ||
.tabs__item { | ||
background-color: #f9fbfc; /* Sets the tab color the same as the file borders */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In an effort to reduce the amount of custom colors used, would we be able to swap this with the var(--color-white)
variable. That color is close enough to white where it shouldn't be too noticeable.
.tabs__item { | ||
background-color: #f9fbfc; /* Sets the tab color the same as the file borders */ | ||
border: 2px solid #ccc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same note about using available color variables here, can we swap all uses of #ccc
with a similar color currently available as a variable?
/* Default color and border for tabs in dark mode */ | ||
[data-theme='dark'] .tabs__item { | ||
background-color: #000000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid using black (not a brand color), can we swap this and anywhere #000 is used with --color-primary-blue
, which is set to #262a38
?
/*Color when hovering over tabs */ | ||
.tabs__item:hover { | ||
background-color: #D95B3B; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#d95b3b
is not an available brand color. Could we swap this with #DE5D43
or #ff694a
? #ff694a
has a variable already, but a new variable would need to be created for #DE5D43
if you go with that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, on hover, the lighter text is still set on the tab while the orange background is applied. This doesn't pass the WCAG accessibility check for contrast link here. The text color may need to be updated on hover to match the color when the tab is active.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And on hover, can we have the border color update to match the background color as well? Similar to how the border updates for the active tab.
|
||
[data-theme='light'] .tabs-container{ | ||
border: 1px solid #000000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In light mode, can we use a lighter color for the border? The var(--ifm-toc-border-color)
variable might be a good option, which is the current border color for the table of contents.
What are you changing in this pull request and why?
Enhancing tabs:
Closes #3889
Checklist