-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add a spinner style for use in interactive blocks. #165
base: trunk
Are you sure you want to change the base?
Conversation
source/wp-content/themes/wporg-parent-2021/sass/blocks/_spinner.scss
Outdated
Show resolved
Hide resolved
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.
Tried it out and the styles aren't being built. Need to add to source/wp-content/themes/wporg-parent-2021/sass/blocks/_index.scss
@@ -0,0 +1,49 @@ | |||
.wporg-spinner { | |||
--local-size: 16px; | |||
--spinner-color: var(--wp--preset--color--blueberry-1, currentColor); |
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 misunderstood this, and tried to change the color of the spinning bit by putting the span in a paragraph block and changing the text color to orange, which didn't work. If we reversed this that would work, but would default to charcoal most of the time. Maybe that's ok, in allowing us to have editor customisation.
Use case would be on a page like SotW, where we show the map of watch parties and there is usually a custom link color.
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 reversed this that would work, but would default to charcoal most of the time.
I don't know if that's possible seeing that currentColor would always be set or inherited so we would never get the blueberry coloring unless explicitly set.
We could do something like this though:
--spinner-color: var(--wp--custom--wporg-spinner--color, var(--wp--preset--color--blueberry-1, currentColor));
We could then set --wp--custom--wporg-spinner--color
to give it a specific color.
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.
--spinner-color: var(--wp--preset--color--blueberry-1, currentColor);
Since this CSS exists in the parent theme, --wp--preset--color--blueberry-1
will always exist. So it will never use currentColor
.
Maybe that's ok, in allowing us to have editor customisation.
Do we need editor customization? This seems like a helper class we might use in another block, where we could override with custom properties in the containing block. That said, I would change it to populate the custom properties from theme.json, for consistency with other block settings.
"settings": {
"custom": {
"wporg-spinner": {
"color": {
"background": "var(--wp--preset--color--black-opacity-15)",
"stroke": "var(--wp--preset--color--blueberry-1)"
}
}
Which would generate --wp--custom--wporg-spinner--color--background
& --wp--custom--wporg-spinner--color--stroke
, so you don't need to set them in the CSS either.
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.
In the past I've found it simple enough to just include the CSS for a spinner in the blocks using it (example on patterns, example on themes). That's not a reason not to do this, but it is a slightly different approach to the spinner.
Anyway, for this PR, I saw a few stylelint issues, I've noted them here. To run the stylelint checks locally, you can use yarn workspace wporg-parent-2021 lint:css
@@ -0,0 +1,49 @@ | |||
.wporg-spinner { |
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.
Minor, but can you move this file to /base/
? /blocks/*
should be for real blocks.
source/wp-content/themes/wporg-parent-2021/sass/blocks/_spinner.scss
Outdated
Show resolved
Hide resolved
source/wp-content/themes/wporg-parent-2021/sass/blocks/_spinner.scss
Outdated
Show resolved
Hide resolved
source/wp-content/themes/wporg-parent-2021/sass/blocks/_spinner.scss
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,49 @@ | |||
.wporg-spinner { | |||
--local-size: 16px; | |||
--spinner-color: var(--wp--preset--color--blueberry-1, currentColor); |
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.
--spinner-color: var(--wp--preset--color--blueberry-1, currentColor);
Since this CSS exists in the parent theme, --wp--preset--color--blueberry-1
will always exist. So it will never use currentColor
.
Maybe that's ok, in allowing us to have editor customisation.
Do we need editor customization? This seems like a helper class we might use in another block, where we could override with custom properties in the containing block. That said, I would change it to populate the custom properties from theme.json, for consistency with other block settings.
"settings": {
"custom": {
"wporg-spinner": {
"color": {
"background": "var(--wp--preset--color--black-opacity-15)",
"stroke": "var(--wp--preset--color--blueberry-1)"
}
}
Which would generate --wp--custom--wporg-spinner--color--background
& --wp--custom--wporg-spinner--color--stroke
, so you don't need to set them in the CSS either.
source/wp-content/themes/wporg-parent-2021/sass/blocks/_spinner.scss
Outdated
Show resolved
Hide resolved
I noticed these using the stylelint extension for VSCode too |
This PR converts the javascript spinner from the google-maps blocks to be used in child themes.
Screenshots
How to test the changes in this Pull Request:
<span class="wporg-spinner"></span>