Spinner: Enforce no background. #49695
Conversation
jasmussen
added
[Type] Enhancement
| @@ -26,6 +26,8 @@ export const StyledSpinner = styled.svg` | |||
| position: relative; | |||
| color: ${ COLORS.ui.theme }; | |||
| overflow: visible; | |||
| opacity: 1; | |||
| background-color: none; | |||
There was a problem hiding this comment.
none is not a valid value for background-color. We can use transparent instead
ciampo
force-pushed
the
try/spinner-enforce-no-background
branch
from
7a8f167 to
0ae2761
Compare
There was a problem hiding this comment.
This PR is good to go 🚀
- Rebased on top of latest
trunk - Swapped
nonefortransparentfor the background color reset - Updated snapshots
- Added missing CHANGELOG entry
A final note: the changes in this PR can only be a temporary fix. The real fix is to avoid style overrides (even more so those using internal class names like components-spinner).
|
Wow, thanks for bringing this home! 🙏 |
| @@ -26,6 +26,8 @@ export const StyledSpinner = styled.svg` | |||
| position: relative; | |||
| color: ${ COLORS.ui.theme }; | |||
| overflow: visible; | |||
| opacity: 1; | |||
| background-color: transparent; | |||
There was a problem hiding this comment.
This could also have been:
opacity: inherit;
background-color: inherit;
which effectively resets the styles that were set by a CSS class rule, and makes them to be inherited from a parent element. Just like if the child element never specified any background-color.
inherit is better in a sense that it doesn't express any opinion about what the background color should be.
There was a problem hiding this comment.
It would inherit the background of its parent element.
If the structure is simply div.components-spinner > svg, then inherit would most likely make it transparent indeed, or at least the same background color as its parent.
But I like transparent in this case, as it is an easily readable statement: this component should always have a transparent background.
There was a problem hiding this comment.
If the structure is simply
div.components-spinner > svg
In the spinner case however the structure is svg.components-spinner, both the class name and the Emotion-generated style are on the same element. It would work.
But never mind, I just wanted to share a possible alternative solution. And in case of background-color, transparent in the end means the same thing as inherit, just on a different conceptual level: "inherit the color of the layer that's under this one"
There was a problem hiding this comment.
Oh you're right, my mistake. But it would still inherit the background of its parent, might have its own background set. It would be invisible unless it was a tiling image, which would be unlikely, but nevertheless, not quite as explicit as "transparent".
There was a problem hiding this comment.
Oh, you're right, too, about the tiling images etc. background-color is not inherited by default, as that would copy the style from the parent, and we don't want to do that. It would break gradients, tiles etc. The right default for background-color is transparent.
It's initial that is the real alternative solution that works:
opacity: initial;
background-color: initial;
References: * WordPress/gutenberg#49695 * WordPress/gutenberg#37551 * p1683530650412459-slack-C0299DMPG
What?
In some environments, or when you have certain plugins active, the Spinner component can inherit a legacy background color from other spinners that share the
components-spinnerclass name. In those cases, they might look like this:... when they should look like this:
This PR adds an explicit "no background" rule, that should at lest prevent the CSS bleed.
Here's an example of CSS that might get inherited:
Testing Instructions
If you're able to test in a Calypso environment, you should see no gray background behind the spinner.