search

StackExchange / Stacks

Stack Overflow’s Design System
https://stackoverflow.design
MIT License
607 stars 89 forks source link

refactor(topbar): use pseudo-private custom properties #1622

Closed dancormier closed 6 months ago

dancormier commented 7 months ago

STACKS-556

This PR is a refactor of the topbar component styles to use our pseudo-private custom properties structure.

I'm considering this PR blocked by https://github.com/StackExchange/Stacks/pull/1624.

Note Some Core Less files directly reference .s-topbar__dark and .s-topbar__light, so this PR includes those styles outside of the .s-topbar block. Eventually, Core should be updated to not act as though these styles are exported.

Note include tests for custom theme properties

netlify[bot] commented 7 months ago

Deploy Preview for stacks ready!

Name Link
Latest commit 04745a74daa222cd6cd5d0f6b35d8370c95aa02f
Latest deploy log https://app.netlify.com/sites/stacks/deploys/65dd103341b1a800085f0d25
Deploy Preview https://deploy-preview-1622--stacks.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

giamir commented 6 months ago

Out of scope for v2.1.0

dancormier commented 6 months ago

Out of curiosity: did the visual regression tests helped you during the refactor? Did they catch anything?

Yes!

I introduced a minor regression in the course resolving merge conflicts which were caught when I ran the visual regression tests. The tests also gave me a bit more confidence in my changes and saved me lots of time so I could focus on testing the things not considered by the tests (element interactive states and custom theming, for example).

dancormier commented 6 months ago

@giamir thank you for reviewing these changes. I know that it's a tricky one to review but I appreciate you taking the time to do so. Since your review, I expanded the visual test slightly to include a topbar with custom theme variables set to give us more confidence that those variables will work as expected.