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
base: main
Are you sure you want to change the base?
Example color theme improvements #5087
Conversation
…ts / restrictions
There are some unexpected Percy changes, it looks like something I changed in the SCSS has had some unintended consequences. Investigating now |
Removed example sass from vanilla bundle and now there are no percy diffs. |
2c56fbe
to
9ee4632
Compare
9ee4632
to
9f766dc
Compare
There was a problem hiding this 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.
scss/docs/example.scss
Outdated
|
||
// stylelint-disable selector-max-type -- examples can use type selectors | ||
body { | ||
margin: 1rem; | ||
} | ||
// stylelint-enable selector-max-type | ||
|
||
.u-example-controls { |
There was a problem hiding this comment.
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`; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
@lyubomir-popov Your feedback is now in the demo; see button for example |
position: fixed; | ||
right: 0; | ||
|
||
.u-theme-toggle__button { |
There was a problem hiding this comment.
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).
Done
More limited version of #5086 that achieves #5086 goals without modifying the back-end or modifying example templates.
Fixes #5069, WD-11110
QA
div.u-example-controls
) is positioned to the bottom right of the page, not its contents.theme
has been set to your selected theme.theme
query parameter is present in your URL.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:
Feature 🎁
,Breaking Change 💣
,Bug 🐛
,Documentation 📝
,Maintenance 🔨
.package.json
should be updated relative to the most recent release, following semver convention:Screenshots