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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

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

// stylelint-disable selector-max-type -- examples can use type selectors
body {
margin: 1rem;
}
// stylelint-enable selector-max-type

.u-example-controls {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit picking a bit (as it doesn't matter that much in an internal tool), but it seems more like "component" than "utility", so we could go with p-… prefix instead (because I guess there is no point yet to introduce any internal vanilla prefix that would be different).

* @returns {string}
*/
function convertThemeNameToJsTogglerClassName(themeName) {
return `js-${themeName}-theme-toggle`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this class needed at all? It doesn't seem to be used to attach functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jmuzina any need for that still? You seem to be attaching click event via u-theme-toggle__button class name (which is debatable by itself), but seems like we don't really use this js- classes anywhere?

@jmuzina
Copy link
Member Author

jmuzina commented May 20, 2024

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

position: fixed;
right: 0;

.u-theme-toggle__button {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we need a class for it, I guess it would be good to follow the convention of BEM and call it p-example-controls__button, not pretend it's a "utility".

Also, as the label is set in JS, maybe we can remove it alltogether and capitalise the first letter in JS (although it's nice to keep this presentational code in CSS).

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
4 participants