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

Global styles: add option to remove site-wide theme background image #61998

Merged
merged 1 commit into from
May 27, 2024

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented May 27, 2024

Dev Note 📓

#59354 (comment)

What?

Part of:

Hello.

This PR implements @andrewserong's idea and adds functionality to remove a theme's default, site-wide background image in the editor.

See: #61271 (comment)

Why?

Check it out:

2024-05-27.12.00.28.mp4

There's no way to remove a background image if it's been defined in theme.json.

How?

By adding a "Remove" option to the image dropdown that sets the value of background.backgroundImage to "none", when:

  1. A user value has not been set, and
  2. There's a theme.json value

Testing Instructions

  1. Define a background image for your site in theme.json
  2. Head over to the site editor Global Styles > Layout
  3. Under Background, click on the image control and then click on "Remove"
  4. The background image should disappear! Save and check the frontend. The value of background-image should be "none".
  5. In the site editor again, reset the background image value by clicking on the image control and then click on "Reset"
  6. You should see the default background image again.

Here's some test theme.json!

{
	"$schema": "../../schemas/json/theme.json",
	"version": 3,
	"settings": {
		"appearanceTools": true
	},
	"styles": {
		"background": {
			"backgroundImage": {
				"url": "https://images.pexels.com/photos/22484288/pexels-photo-22484288/free-photo-of-the-circular-stone-terraces-of-the-inca-ruins.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=2"
			}
		}
	}
}

Screenshots or screencast

2024-05-27.11.50.07.mp4

…-wide background image in the editor by settings the value to `background-image: none`
@ramonjd ramonjd added [Type] Enhancement A suggestion for improvement. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels May 27, 2024
@ramonjd ramonjd self-assigned this May 27, 2024
@ramonjd ramonjd requested a review from ellatrix as a code owner May 27, 2024 02:05
Copy link

github-actions bot commented May 27, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@@ -279,6 +281,23 @@ function BackgroundImageToolsPanelItem( {

const hasValue = hasBackgroundImageValue( style );

const closeAndFocus = () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

An abstraction since we need this in two places

@@ -83,6 +83,8 @@ export function hasBackgroundSizeValue( style ) {
export function hasBackgroundImageValue( style ) {
return (
!! style?.background?.backgroundImage?.id ||
// Supports url() string values in theme.json.
'string' === typeof style?.background?.backgroundImage ||
Copy link
Member Author

Choose a reason for hiding this comment

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

This is so we can detect strings values with url()

	"styles": {
		"background": {
			"backgroundImage": "url('https://images.pexels.com/photos/22484288/pexels-photo-22484288/free-photo-of-the-circular-stone-terraces-of-the-inca-ruins.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=2')"
		}
	}

onRemove();
} }
>
{ __( 'Remove' ) }
Copy link
Member Author

Choose a reason for hiding this comment

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

Is "Remove" clear enough?

It should only appear where there's a theme.json value AND there's no user style defined - basically so users can remove the default theme background image and nothing else.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's a good question. I think visually it's pretty clear when you open the dropdown by clicking on tile with the image name and thumbnail; in context "Remove" could only be related to that image. But a quick test with VoiceOver shows the experience is pretty flaky if you don't have the visual context: you tab past a menu popup button named "Background options" (this is the only clue you get that you're in a section related to background), then you tab into another menu popup button labeled with the the image file name, plus "Selected image: Untitled". That's a wider problem than this PR addresses though, so I'm not suggesting tying to solve it here 😅

Really tools panels were never accessible to begin with, which is the main issue, but perhaps for this particular instance we could improve the experience by switching the order of file name and "Selected image: Untitled", and maybe changing "Selected image" to "Site background image" (I'm not sure where the "Untitled" is coming from? but maybe we could dispense with it altogether?). But given this is an existing issue it should probably be addressed in a follow-up.

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Code LGTM and it's working well in testing! Plus it's a really useful enhancement, because currently there's no way of getting rid of a theme background image short of editing the theme files 😅

onRemove();
} }
>
{ __( 'Remove' ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's a good question. I think visually it's pretty clear when you open the dropdown by clicking on tile with the image name and thumbnail; in context "Remove" could only be related to that image. But a quick test with VoiceOver shows the experience is pretty flaky if you don't have the visual context: you tab past a menu popup button named "Background options" (this is the only clue you get that you're in a section related to background), then you tab into another menu popup button labeled with the the image file name, plus "Selected image: Untitled". That's a wider problem than this PR addresses though, so I'm not suggesting tying to solve it here 😅

Really tools panels were never accessible to begin with, which is the main issue, but perhaps for this particular instance we could improve the experience by switching the order of file name and "Selected image: Untitled", and maybe changing "Selected image" to "Site background image" (I'm not sure where the "Untitled" is coming from? but maybe we could dispense with it altogether?). But given this is an existing issue it should probably be addressed in a follow-up.

@ramonjd
Copy link
Member Author

ramonjd commented May 27, 2024

Thanks for the feedback and testing @tellthemachines

But a quick test with VoiceOver shows the experience is pretty flaky if you don't have the visual context
we could improve the experience by switching the order of file name and "Selected image: Untitled", and maybe changing "Selected image" to "Site background image" (I'm not sure where the "Untitled" is coming from? but maybe we could dispense with it altogether?). But given this is an existing issue it should probably be addressed in a follow-up.

Good idea. "untitled" is the fallback where the image has no title.

https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/global-styles/background-panel.js#L305

I have a tweak PR going so I can update that over there.

Cheers!

@ramonjd ramonjd merged commit a307229 into trunk May 27, 2024
72 of 73 checks passed
@ramonjd ramonjd deleted the add/background-image-theme-reset branch May 27, 2024 05:46
@github-actions github-actions bot added this to the Gutenberg 18.5 milestone May 27, 2024
@jasmussen
Copy link
Contributor

Good starting point, I'm assuming we can also reset it using the toolspanel mechanism as is the case for the group bg option?

Screenshot 2024-05-27 at 08 23 43

There may be a larger question, across all design-tools and perhaps related to the inheritance visualization work, to improve how items are generally reset to their theme.json definitions. It's ultimately about user customizations. The curious piece here is that you may be resetting an image, and are then surprised to still see an image, expecting there to be no image. The closest equivalent to mimic is likely going to be having an explicit "no background image" option, similar to how we have explicit text-decoration definitions, or explicit "no color" definitions.

@tellthemachines
Copy link
Contributor

I'm assuming we can also reset it using the toolspanel mechanism as is the case for the group bg option?

If the background image is defined by the theme (if comes from the theme.json) then the tools panel reset shouldn't remove it, right? At least I always assumed that "reset" means returning to whatever the theme defaults are.

@jasmussen
Copy link
Contributor

At least I always assumed that "reset" means returning to whatever the theme defaults are.

Global styles should always show all tools in a tools panel, and yes, clicking reset should reset to "default", which would be whatever default theme.json provides.

So I'm not suggesting an action item, I'm mainly thinking through what the longer term solution is for adding an explicit "none" option background images, similar to explicit "no background color" or explicit "no text-decoration" options, for the case where you want to unset a default. What we have in this branch is good, just wondering if there are clearer labels: "remove" and "reset" are a bit confusing. I wonder if "Reset" and "No background image" would be clearer?

@fabiankaegy fabiankaegy added the Needs Dev Note Requires a developer note for a major WordPress release cycle label May 29, 2024
@juanmaguitar juanmaguitar mentioned this pull request May 29, 2024
49 tasks
@ramonjd ramonjd added has dev note when dev note is done (for upcoming WordPress release) and removed Needs Dev Note Requires a developer note for a major WordPress release cycle labels May 30, 2024
carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Jun 4, 2024
…-wide background image in the editor by settings the value to `background-image: none` (WordPress#61998)

Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org>
patil-vipul pushed a commit to patil-vipul/gutenberg that referenced this pull request Jun 17, 2024
…-wide background image in the editor by settings the value to `background-image: none` (WordPress#61998)

Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json has dev note when dev note is done (for upcoming WordPress release) [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants