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

Example color theme improvements #5087

Merged

Conversation

jmuzina
Copy link
Member

@jmuzina jmuzina commented May 10, 2024

Done

More limited version of #5086 that achieves #5086 goals without modifying the back-end or modifying example templates.

Fixes #5069, WD-11110

QA

  • Open demo
  • Open a themed component e.g., table
    • Verify that the color theme switcher is present and uses Segmented Control component.
    • Verify that the baseline grid toggler works.
    • Inspect the element containing the theme switcher & baseline grid control. Verify that the element (div.u-example-controls) is positioned to the bottom right of the page, not its contents.
    • Switch color themes. Verify that the theme has been changed for the example, segmented control state has been updated to reflect the active theme, and the URL query parameter theme has been set to your selected theme.
    • Refresh the browser. Verify that the theme you previously had selected is still active.
    • Small/Medium breakpoints:
      • Verify that the theme switcher & baseline grid control are neatly positioned in the same element & both visible on screen.
      • Verify that the segmented control segments (color themes) are all on the same horizontal line (do not wrap).
  • Open a non-themed component, e.g., Article Pagination.
    • Verify that the color theme switcher is not present & no theme query parameter is present in your URL.
    • Verify that the baseline grid switcher is still present & functional.

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 relesase (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

image

@jmuzina jmuzina added the Documentation 📝 Documentation changes or updates label May 10, 2024
@jmuzina jmuzina self-assigned this May 10, 2024
@webteam-app
Copy link

@jmuzina
Copy link
Member Author

jmuzina commented May 10, 2024

There are some unexpected Percy changes, it looks like something I changed in the SCSS has had some unintended consequences. Investigating now

@jmuzina
Copy link
Member Author

jmuzina commented May 10, 2024

Removed example sass from vanilla bundle and now there are no percy diffs.

@jmuzina jmuzina force-pushed the example-color-theme-improvements-limited branch from 2c56fbe to 9ee4632 Compare May 10, 2024 18:49
@jmuzina jmuzina force-pushed the example-color-theme-improvements-limited branch from 9ee4632 to 9f766dc Compare May 10, 2024 18:57
@jmuzina jmuzina changed the title Example color theme improvements - limited scope Example color theme improvements May 10, 2024
@jmuzina jmuzina marked this pull request as ready for review May 10, 2024 19:42
Copy link
Contributor

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

I think there are some leftovers from previous implementation that may be not needed anymore.

templates/static/js/example-tools.js Outdated Show resolved Hide resolved
scss/docs/example.scss Outdated Show resolved Hide resolved
templates/static/js/example-tools.js Show resolved Hide resolved
@jmuzina
Copy link
Member Author

jmuzina commented May 20, 2024

@lyubomir-popov Your feedback is now in the demo; see button for example

scss/docs/example.scss Outdated Show resolved Hide resolved
@jmuzina jmuzina force-pushed the example-color-theme-improvements-limited branch from 67bbe5a to 4dce656 Compare May 24, 2024 15:03
@jmuzina jmuzina force-pushed the example-color-theme-improvements-limited branch from 4dce656 to 16e4ec3 Compare May 24, 2024 15:13
@pastelcyborg
Copy link
Contributor

Not directly related to the color theming toggle functionality, but I notice some components still don't fully support all the themes - for example:

https://vanilla-framework-5087.demos.haus/docs/examples/patterns/tables/table-colored-rows?theme=dark

Does this affect our desire to add this theme toggle right now? Seems like maybe it should be held off until all the components fully support each theme?

@jmuzina
Copy link
Member Author

jmuzina commented May 24, 2024

Does this affect our desire to add this theme toggle right now? Seems like maybe it should be held off until all the components fully support each theme?

@pastelcyborg We're not adding the theme toggle - it's currently in production. The problem with it is that it doesn't reflect the active state (the selected color theme is not shown on the theme selector).

See comment by Bartek for context

@bartaz
Copy link
Contributor

bartaz commented May 28, 2024

Does this affect our desire to add this theme toggle right now? Seems like maybe it should be held off until all the components fully support each theme?

The theme switcher is already there, this PR only makes it better.
It is our goal in longer term to figure out how this is supposed to work across all the examples which may have some hardcoded colours now.

@bartaz
Copy link
Contributor

bartaz commented May 28, 2024

There are some z-index issues (with notifications for example).
I guess we should increase the z-index of this toolbar to be above anything else.

image

https://vanilla-framework-5087.demos.haus/docs/examples/layouts/docs

Even more visible here:
https://vanilla-framework-5087.demos.haus/docs/examples/templates/z-index?theme=light

Copy link
Contributor

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

LGTM, thanks!

@bartaz bartaz merged commit b1d6513 into canonical:main May 29, 2024
6 checks passed
@jmuzina jmuzina deleted the example-color-theme-improvements-limited branch June 5, 2024 16:36
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.

Examples theme picker does not reflect selected theme
5 participants