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

Add reset/clear to the group block background image control #54319

Closed
richtabor opened this issue Sep 8, 2023 · 11 comments · Fixed by #54341
Closed

Add reset/clear to the group block background image control #54319

richtabor opened this issue Sep 8, 2023 · 11 comments · Fixed by #54341
Assignees
Labels
[Block] Group Affects the Group Block Needs Design Feedback Needs general design feedback. [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@richtabor
Copy link
Member

I agree with @jameskoster's comment on resetting. I expected to find the "Reset" control within the new group block's background image panel. You can reset it within the tools panel, but it feels necessary (and familiar, i.e. the Site Logo block) to have it here.

Another note (related to #53997), we should consistent on "Clear" vs. "Reset".

We're using "Clear" within the color palette popover, but "Reset" within the tools panel. Same follows for the Cover block's "Clear media" button.

Current:
CleanShot 2023-09-08 at 17 06 19

Expected:
CleanShot 2023-09-08 at 17 05 13

@hanneslsm
Copy link

Another note (related to #53997), we should consistent on "Clear" vs. "Reset".

I wrote a comment regarding this and posted it in the related issue: #46859 (comment)

@andrewserong andrewserong self-assigned this Sep 10, 2023
@andrewserong
Copy link
Contributor

Thanks for opening this Rich! I'll look into a follow-up.

@jameskoster
Copy link
Contributor

We probably need to consider both clear and reset:

– Clear removes the image, IE background-image: none.
– Reset removes the locally set image if there is one and the default from global styles (if there is one) is inherited.

If a background image is applied globally and locally, then its conceivable that there should be separate clear/reset actions when editing in the local context.

@richtabor
Copy link
Member Author

the default from global styles (if there is one) is inherited.

I wouldn't think this would be common (or likely even?).

@richtabor
Copy link
Member Author

richtabor commented Sep 11, 2023

Or perhaps "Remove" covers both bases.

It removed this application of image (or even color, font, duotone, etc). It just falls back to whatever is in global styles, not background-image: none, just like color does.

@jameskoster
Copy link
Contributor

I wouldn't think this would be common (or likely even?).

Probably not common, but if it is made possible to add a background image globally then we need to consider it.

A single "Remove" could work. First click would reset to global default, a second click would effectively set background-image: none locally. Not sure if the extra click is a deal-breaker.

I figured it'd make sense to plot the patterns proposed in #49278 against this use case and here's what we get:

bgimage

I reckon this works well, though I acknowledge we're not quite ready to implement it yet.

@richtabor
Copy link
Member Author

Probably not common, but if it is made possible to add a background image globally then we need to consider it.

I don't think it should tbh.

"Reset" and "Clear" are similar enough that most folks won't understand the nuance here (we use both inconsistently already). And that exposes functionality we don't support elsewhere — i.e. if you set a background color to the group block, you can't simply "remove" that color in the editor.

@andrewserong
Copy link
Contributor

Good discussion, folks! Over in #54341 the proposed code change is to clear out the local background image via the Reset menu item, but without imposing any background-image: none kind of rules. I imagined we'd look at the nuance of overriding / clearing out things set in global styles for if and when we implement background images in global styles, which is probably a little further off (e.g. after WP 6.4).

@richtabor
Copy link
Member Author

Over in #54341 the proposed code change is to clear out the local background image

And just to be clear (pun unintended, but welcomed), that's with a proposed label of "Reset"?

@andrewserong
Copy link
Contributor

And just to be clear (pun unintended, but welcomed), that's with a proposed label of "Reset"?

I just realised I used the word "clear" in my sentence, too! 😆

Yes, that PR is with the proposed label of "Reset" 🙂

@jordesign jordesign added the [Type] Enhancement A suggestion for improvement. label Sep 12, 2023
@jameskoster
Copy link
Contributor

"Reset" and "Clear" are similar enough that most folks won't understand the nuance here

Then we should make that nuance clearer (continuing the pun). If you've added a background image to a group locally then these are both valid actions:

  • I want this group to have no background image.
  • I want this group to use the default background image.

We could make it sequential, IE the first "reset" resets to the global default, and a subsequent "reset" sets background-image: none. I'm not a big fan because removing takes so many more clicks, but it's an option.

Alternatively the labels could be more explicit. Perhaps "Reset to default" (which is only available when there is a default) and "Remove"?

And that exposes functionality we don't support elsewhere — i.e. if you set a background color to the group block, you can't simply "remove" that color in the editor.

Color is a bit different because you can effectively 'remove' a background color by setting the transparency to 0. I suppose you could upload a transparent background image to fudge background-image: none; but that's not very intuitive 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Group Affects the Group Block Needs Design Feedback Needs general design feedback. [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants