Background image: Add has-background classname when background image is applied #57495
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewserong
added
[Type] Code Quality
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/block-supports/background.php ❔ phpunit/block-supports/background-test.php |
|
Size Change: +126 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
ramonjd
reviewed
Jan 4, 2024
There was a problem hiding this comment.
Looks good when adding a group block with background image and color.
Also, I can't see any probs when switching to classic theme for an already-saved-in-block-theme post. 👍🏻
In both the editor and site frontend there should still be only one has-background classname on the Group block.
I'm still seeing two has-background classes in the editor. I think coming from color??
Is it me?
| */ | ||
| export function getBackgroundImageClasses( style ) { | ||
| return classnames( { | ||
| 'has-background': hasBackgroundImageValue( style ), |
There was a problem hiding this comment.
Is this the same as hasBackgroundImageValue( style ) ? 'has-background' : '' ?
There was a problem hiding this comment.
Yep, that'd be simpler! I was originally thinking of potentially adding more classnames, so grabbed classnames() but until we have a need for it, I reckon your idea's better 👍
Not just you! I'll dig into it. I wasn't seeing it at first when I put this PR together, but I very well might have been going too quickly when looking at the editor side of things. It looks like the current logic possibly doesn't wind up doing deduping on the classname. I'll see what's going on 🙂 |
Maybe it's time for the style engine to take care of classes in JS as well |
…ound color classnames
|
@ramonjd, I've just pushed an update that I think resolves it 🤞 TIL that classnames doesn't automatically dedupe in our Also, since each block support is running in isolation, there's no way (that I'm aware of) to check within one block support whether a classname has been added in another block support. Since we're sharing the same classname as is used by the background color, I thought it might be logical to combine logic there, and place the backgroundImage classname output in the Let me know if all this feels too convoluted! It took a little while to explain in this comment, but the amount of code looks fairly minimal, at least 😄 |
ramonjd
approved these changes
Jan 5, 2024
There was a problem hiding this comment.
Thanks for the updates @andrewserong
LGTM
Checked:
- site and post editors
- switching to classic theme (background and classes persist, and the background controls aren't there)
- no classname duplication
|
Thanks for the re-review! 🙇 |
andrewserong
deleted the
try/add-has-background-classname-to-background-image-support
branch
This was referenced Jan 7, 2024
|
Backported in https://core.trac.wordpress.org/ticket/60175 |
getdave
added
the
Backported to WP Core
|
✅ I updated this PR with the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes: #56261
Add the
has-backgroundclassname when a background image is appliedWhy?
A background image is one kind of background, and we already output
has-backgroundif a background color or gradient is applied to a block. For consistency, let's also output it if a background image is set without any background colors.How?
colorsupport's code where thehas-backgroundclassname is handled for the editor. This ensures we only output the classname when it hasn't already been added to the classnames list.has-backgroundclassname if background image styles are to be output.Testing Instructions
has-backgroundclassname.has-backgroundclassname.has-backgroundclassname on the Group block.Testing Instructions for Keyboard
Screenshots or screencast
Editor
Site frontend