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

Theme accent color #5328

Merged
merged 4 commits into from
Aug 30, 2024
Merged

Theme accent color #5328

merged 4 commits into from
Aug 30, 2024

Conversation

jmuzina
Copy link
Member

@jmuzina jmuzina commented Aug 27, 2024

Done

Themes the accent color by adding a new dark-theme accent color.

Fixes WD-11879

QA

  • Open demo
  • Verify accented color is correct and easy to read on all themes.

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix release (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

Screenshots

Screenshot 2024-08-28 at 3 42 16 PM

@webteam-app
Copy link

@jmuzina jmuzina marked this pull request as ready for review August 27, 2024 22:06
scss/_settings_colors.scss Outdated Show resolved Hide resolved
@bartaz
Copy link
Member

bartaz commented Aug 28, 2024

TBH I feel like this PR was opened a bit too soon. From the conversation on the issue it doesn't seem we had a decision to theme the colour, or what new value should be.

Currently the accent colour is not themed (has the same value in both themes). We could keep this funtionality, but add it to theme variables (both dark and light versions referencing the same $color-accent value). Which could be a quick update to review (as it doesn't introduce any visual changes).

Now, we are opening the discussion in introducing a new colour for the dark theme, which may prolong the review.

@jmuzina
Copy link
Member Author

jmuzina commented Aug 28, 2024

Will close this but keep the branch around, and use it if/when the design decisions have been made

@jmuzina jmuzina closed this Aug 28, 2024
@jmuzina jmuzina reopened this Aug 28, 2024
@jmuzina
Copy link
Member Author

jmuzina commented Aug 28, 2024

Design has decided on #70bbc2 - added it here!

@bartaz bartaz added Review: Percy +1 and removed Review: Percy needed This PR needs a review of Percy for visual regressions labels Aug 30, 2024
Copy link
Member

@bartaz bartaz 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.

@bartaz
Copy link
Member

bartaz commented Aug 30, 2024

Color contrast is fine
image

@jmuzina jmuzina merged commit 308a5c9 into canonical:main Aug 30, 2024
19 checks passed
@jmuzina jmuzina deleted the theme-accent-color branch August 30, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants