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(dropdowns): new light/dark mode colors #1816

Merged
merged 14 commits into from
Jun 5, 2024
Merged

Conversation

jzempel
Copy link
Member

@jzempel jzempel commented May 23, 2024

Description

Verified Combobox with all combinations of: startIcon & endIcon, placeholder, inputValue, focusInset, validation, isMultiselectable with tags & more than maxTags, and isDisabled.

Verified Option with all combinations of: icon, isSelected, type, isDisabled, and Option.Meta.

Verified OptGroup with label & icon.

Ensured that all Option combinations were inherited as expected by Menu > Item.

Detail

There are two outstanding questions for design re:

  • icon + meta colors for <Option type="add">
  • icon + meta colors for <Option isDisabled>

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

@jzempel jzempel requested a review from a team as a code owner May 23, 2024 20:00
@coveralls
Copy link

coveralls commented May 23, 2024

Coverage Status

coverage: 96.039% (-0.04%) from 96.083%
when pulling 75b8192 on jzempel/recolor-dropdowns
into e3f1533 on next.

@lucijanblagonic
Copy link

icon + meta colors for <Option isDisabled>

image

Use the foreground/disabled for all three when it's disabled: icon, label, meta

icon + meta colors for <Option type="add">

I would align add and danger here. Original reasoning was to make danger stand out. But after the changes we agreed on the focus state (blue indicator for danger), I think it makes sense to have unified treatment for meta.

image

For both Add and Danger, it's the same approach.

  • meta -> foreground/subtle
  • label + icon -> foreground/primary and foreground/danger, depending on the type

The rest looks great. 🙌

@jzempel
Copy link
Member Author

jzempel commented May 29, 2024

thanks @lucijanblagonic .. the PR has been updated with the finalized color design decisions.

Copy link
Contributor

@geotrev geotrev left a comment

Choose a reason for hiding this comment

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

Just the one open question. 🚀

const colorStyles = (props: IStyledValueProps) => {
const foregroundColor = props.isPlaceholder && getColorV8('neutralHue', 400, props.theme);
const colorStyles = ({ theme, isPlaceholder }: IStyledValueProps) => {
const foregroundColor = isPlaceholder && getColor({ theme, variable: 'foreground.disabled' });
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this return undefined or does styled-components evaluate properties as falsey to omit them? I see both around the codebase so just confirming.

Copy link
Member Author

Choose a reason for hiding this comment

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

styled-components does the right thing

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.

thanks for making those menu item changes! combobox and menu focus treatments LGTM!

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 looks good. 🙌

@jzempel jzempel merged commit 142286c into next Jun 5, 2024
5 checks passed
@jzempel jzempel deleted the jzempel/recolor-dropdowns branch June 5, 2024 14:43
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.

7 participants