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

feat(tabs): new light/dark mode colors #1843

Merged
merged 3 commits into from
Jun 26, 2024
Merged

feat(tabs): new light/dark mode colors #1843

merged 3 commits into from
Jun 26, 2024

Conversation

ze-flo
Copy link
Contributor

@ze-flo ze-flo commented Jun 21, 2024

Description

Adds new light/dark mode colors to Tabs

Detail

  • Adds new light/dark mode colors to Tabs
  • Refactors using transient props

Checklist

  • 👌 design updates will be Garden Designer approved (add the designer as a reviewer)
  • 🌐 demo is up-to-date (npm start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 🤘 renders as expected with Bedrock CSS (?bedrock)
  • 💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
  • ♿ tested for WCAG 2.1 AA accessibility compliance
  • 📝 tested in Chrome, Firefox, Safari, and Edge

@ze-flo ze-flo marked this pull request as ready for review June 21, 2024 02:39
@ze-flo ze-flo requested a review from a team as a code owner June 21, 2024 02:39
@ze-flo ze-flo requested review from lucijanblagonic and steue June 21, 2024 02:40
const borderInlineColor = $isVertical ? borderColor : undefined;

const selectedColor = getColor({ theme, variable: 'foreground.primary' });
const foregroundColor = $isSelected ? selectedColor : 'inherit';
Copy link
Contributor Author

@ze-flo ze-flo Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steue @lucijanblagonic The default foreground color was originally set to inherit the TabsList's foreground.default. This has the benefit of showing a clear distinction between default and disabled tabs.

Screenshot 2024-06-20 at 4 33 14 PM
Screenshot 2024-06-20 at 4 33 02 PM

Should we keep it as such or use foreground.subtle as shown in Figma instead?

CC: @jzempel

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inheriting feels OK here as long as the intent is preserved by the variable (e.g., disabled vs selected vs. default (inherited)). Hopefully I'm understanding this correctly. haha

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ze-flo Yes, the foreground.default is the preferred color here. We corrected this in design, and we left a note for making the change in the inventory.

@lucijanblagonic
Copy link

lucijanblagonic commented Jun 24, 2024

There seems to be a flash of black, when the selection indicator is changed. This is only visible for users who rely on keyboard to change the tab. I am assuming that the foreground.primary and hover mask the black for pointer users. In dark mode, the flash is in white, foreground.default probably.

tabs-flash

image

@jzempel
Copy link
Member

jzempel commented Jun 24, 2024

There seems to be a flash of black, when the selection indicator is changed. This is only visible for users who rely on keyboard to change the tab. I am assuming that the foreground.primary and hover mask the black for pointer users. In dark mode, the flash is in white, foreground.default probably.

The only reason we're not seeing this in the current is due to the fact that the contrast between the inherited foreground and primary blue is not as high. We might need to question whether the border transition ease is serving us well here 🤷 (or could a very slight transition-delay fix things?).

Copy link

@lucijanblagonic lucijanblagonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything else looks OK.

@ze-flo
Copy link
Contributor Author

ze-flo commented Jun 25, 2024

There seems to be a flash of black, when the selection indicator is changed. This is only visible for users who rely on keyboard to change the tab. I am assuming that the foreground.primary and hover mask the black for pointer users. In dark mode, the flash is in white, foreground.default probably.

The only reason we're not seeing this in the current is due to the fact that the contrast between the inherited foreground and primary blue is not as high. We might need to question whether the border transition ease is serving us well here 🤷 (or could a very slight transition-delay fix things?).

The last commit removes the color flash when using the keyboard. ✅

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 🙌

Copy link

@steue steue 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! ty!

@ze-flo ze-flo merged commit 4a4b92d into next Jun 26, 2024
5 checks passed
@ze-flo ze-flo deleted the ze-flo/tabs-recolor branch June 26, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants