Skip to content
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

Open
wants to merge 2 commits into
base: current
Choose a base branch
from
Open

Adding tab borders #6593

wants to merge 2 commits into from

Conversation

matthewshaver
Copy link
Contributor

What are you changing in this pull request and why?

Enhancing tabs:

  • Places borders around individual tabs
  • Sets a unique background color for clicked and hovered tabs
  • Puts a border around the entire table area so it's clear where tabbed content ends

Closes #3889

Checklist

  • I have reviewed the Content style guide so my content adheres to these guidelines.
  • The topic I'm writing about is for specific dbt version(s) and I have versioned it according to the version a whole page and/or version a block of content guidelines.
  • I have added checklist item(s) to this list for anything anything that needs to happen before this PR is merged, such as "needs technical review" or "change base branch."
  • The content in this PR requires a dbt release note, so I added one to the release notes page.

@matthewshaver matthewshaver requested a review from a team as a code owner December 5, 2024 00:52
Copy link

vercel bot commented Dec 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs-getdbt-com ✅ Ready (Inspect) Visit Preview Dec 12, 2024 3:26pm

@github-actions github-actions bot added size: medium This change will take up to a week to address Docs team Authored by the Docs team @dbt Labs labels Dec 5, 2024
Copy link
Contributor

@mirnawong1 mirnawong1 left a 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 !

Copy link
Collaborator

@JKarlavige JKarlavige left a 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!

Copy link
Collaborator

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 */
Copy link
Collaborator

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;
Copy link
Collaborator

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;
Copy link
Collaborator

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;
Copy link
Collaborator

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.

Copy link
Collaborator

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.
image

Copy link
Collaborator

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;
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs team Authored by the Docs team @dbt Labs size: medium This change will take up to a week to address
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: Add UI distinction to tell tabs are tabs
3 participants