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.

Sign up for GitHub

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

Spinner: Enforce no background. #49695

Merged
merged 5 commits into from
Apr 15, 2023
Merged

Spinner: Enforce no background. #49695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8e63ebf
Spinner: Enforce no background.
jasmussen Apr 10, 2023
49f7f59
Include opacity.
jasmussen Apr 10, 2023
1fae855
CHANGELOG
ciampo Apr 15, 2023
f48ca55
Use `transparent` to reset the background
ciampo Apr 15, 2023
0ae2761
Update snapshots
ciampo Apr 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

- `DropZone`: Smooth animation ([#49517](https://github.com/WordPress/gutenberg/pull/49517)).
- `Navigator`: Add `skipFocus` property in `NavigateOptions`. ([#49350](https://github.com/WordPress/gutenberg/pull/49350)).
- `Spinner`: add explicit opacity and background styles ([#49695](https://github.com/WordPress/gutenberg/pull/49695)).

### Bug Fix

Expand Down
2 changes: 2 additions & 0 deletions packages/components/src/spinner/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ export const StyledSpinner = styled.svg`
position: relative;
color: ${ COLORS.ui.theme };
overflow: visible;
opacity: 1;
background-color: transparent;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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;

`;

const commonPathProps = css`
Expand Down
2 changes: 2 additions & 0 deletions packages/editor/src/components/post-publish-panel/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,8 @@ exports[`PostPublishPanel should render the spinner if the post is being saved 1
position: relative;
color: var(--wp-components-color-accent, var(--wp-admin-theme-color, #007cba));
overflow: visible;
opacity: 1;
background-color: transparent;
}

.emotion-2 {
Expand Down