canonical / vanilla-framework

From community websites to web applications, this CSS framework will help you achieve a consistent look and feel.
https://vanillaframework.io
GNU Lesser General Public License v3.0
798 stars 163 forks source link

Update Switch component with new theme color variables #5185

Closed pastelcyborg closed 5 days ago

pastelcyborg commented 1 week ago

Done

Fixes WD-11873

QA

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:

webteam-app commented 1 week ago

Demo

Jenkins

demos.haus

pastelcyborg commented 1 week ago

Please note that I made some assumptions here due to the following:

jmuzina commented 1 week ago

I'm noticing that the labeled Percy job is not running, even though it ran in the QA I ran when testing that label workflow. It's possible either Github changed something with how contains works or that something about this PR is different than the ones that I tested. Investigating it now.

EDIT: PR is up: https://github.com/canonical/vanilla-framework/pull/5188

lyubomir-popov commented 1 week ago

Looks good! Couple of nit picks:

  • We don't have a themed focus color for links, so I left this the default focus color
  • can we please use pure black for all shadows ($color-x-dark)
  • can we please use #9cf for focus in the dark theme image
pastelcyborg commented 1 week ago

Looks good! Couple of nit picks:

  • We don't have a themed focus color for links, so I left this the default focus color
  • can we please use pure black for all shadows ($color-x-dark)
  • can we please use #9cf for focus in the dark theme image

Cool, thanks @lyubomir-popov! The shadow update is trivial, but for the focus color, I'll probably need to create a new theme color. Any thoughts about this, @bartaz?

bartaz commented 1 week ago

Cool, thanks @lyubomir-popov! The shadow update is trivial, but for the focus color, I'll probably need to create a new theme color. Any thoughts about this, @bartaz?

If we need one, than we need one. For now I assumed focus colour has good enough contrast regardless of the theme. But we can surely add one, $colors--theme--color-focus I guess, (and consistently the CSS variable), with the values that are needed.

And yes, as you mentioned in MM, there is the %vf-focus that is used across the board that would need updating as well (possibly separately).

Maybe it would actually be worth doing a separate PR for focus colour theming, and only then update this one?

jmuzina commented 1 week ago

Percy fix PR was merged and Percy has run. Percy build is at https://percy.io/bb49709b/vanilla-framework/builds/34958676/changed/1910563673?browser=chrome&browser_ids=56&group_snapshots_by=similar_diff&subcategories=unreviewed%2Cchanges_requested&viewLayout=side-by-side&viewMode=new&width=1280&widths=375%2C1280

pastelcyborg commented 1 week ago

Switching this back to draft, since we talked about doing the focus color variable work prior to this.

jmuzina commented 6 days ago

Blocked pending #5192

pastelcyborg commented 6 days ago

This is once again ready for review now that #5192 has been merged.

bartaz commented 5 days ago

Code looks good, but let's rebase it now, that the percy combined examples have been merged and see how that runs.