[Site Logo]: Add option to remove/clear logo from the block #34820
Conversation
stacimc
added
[Block] Site Logo
|
Size Change: +5.07 kB (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
👋 @creativecoder, I've just added you as a reviewer since you've worked on Site Logo recently, but let me know if there's someone else I should ping in addition/instead :) |
|
@shaunandrews @jasmussen Should this be an icon for removing an image? |
|
Nice work @stacimc, and thanks again for detailed test instructions, it makes testing so much easier! The behaviour within the block and full site editing is working well for me, but I ran into a couple of issues with the Customizer vs block settings for the logo. I was testing with TwentyTwenty theme. Here's the steps I used to reproduce: Removing block doesn't appear to override Customizer's setting
Updating the logo from the block, and then removing it reverts the logo back to the one set by the Customizer
I'm wondering if having the Remove button in the toolbar could potentially cause confusion for a user wishing to remove the block, but not the site logo altogether? Would it work to move the Remove button to a sidebar panel where we can have explanatory text that this will remove the logo altogether? I imagine the task of removing the logo image is something that a user would do with less frequency than removing the block itself, so I'd guess having the button be slightly harder to reach for might not be too much of a problem. We could also add an additional Replace button there if it would look strange to have the Remove button on its own. |
|
For the issues I ran into above, I think there's a clue in #33179, which appears to have removed the
If it adds too much complexity to add back in that sync function, I wonder if we can fire an update to the |
|
I've missed this button, thank you for building it. I took the PR for a spin and saw it working as intended with adding and removing the logo from the toolbar:
It's mighty convenient and feel like the appropriate place for the button.
I had the same instinct because we don't yet have a good pattern for two text-only buttons side by side like they are here. I made some quick explorations:
I don't have a clear favorite of the above, though the simplicity of option 2 is probably a good starting point: it simply increases the spacing between two text-only items. That change might end up being complex, though, due to the gnarly CSS of the block toolbar, and speaking of gap, it suggests we can now refactor the block toolbar to use that for spacing. Worth opening a separate ticket for? |
Thanks for pointing this out @andrewserong, not sure how I missed that in my testing. I've pushed a fix for this, and updated the testing instructions with an important point -- basically you'll need to manually disable the similar site-logo code in Core (
I want to say this is probably fine, since the button here doesn't cause the block to be removed at the same time as deleting the media -- so the user gets immediate visual feedback that something else has happened. The change also doesn't persist unless they Publish/Update the post.
Definitely agree that the user should get that explanatory text before potentially making unwanted changes across their site. I think this might be okay though, because when they save a post with modifications to the Site Logo (or likewise in the site editor) they'll get the appropriate warning in the Save message, and be prompted specifically to save the Site Logo separate from the rest of the post:
I don't have a strong opinion about where it should go -- it seems reasonable to group it with For now I've updated the button to an icon as it's the simplest modification, but I can play around with this a bit more to try to wrangle the toolbar styling a bit. |
It seems reasonable to put it in the inspector as a secondary action. But it would likely need its own share of work in order to ensure the button is supported by enough context to make sense. For example the replace button in the block toolbar works because it's shown in context of the visual itself. So if we were to have a "Remove" button in the inspector, we'd likely want to also show a copy of the image there, complete with a duplicate "replace" button as well — a little like how the Cover block does a focal point picker. Happy to create a mockup if need be? |
|
Thanks for the extra context @stacimc and fixing up the issue. I ran out of time to test this again today, but I'll give it another spin tomorrow. I don't feel too strongly about where the Remove button goes, either, and I think your point that the user will still need to confirm when they update the post helps mitigate the concern about the potential danger of it being in the toolbar. Joen makes a very good point about the additional complexity needed to have the button make sense in the block inspector, so perhaps it's worth keeping it in the toolbar for the time being and we can come back to it as a follow-up if we feel like it could use the extra work? |
Agree this is a good place to start for this PR. |
There was a problem hiding this comment.
Great work @stacimc! This is testing well for me now:
✅ Simple flow is working correctly
✅ Logo persistence is testing well in TT1-blocks
✅ Customizer syncing is now working correctly (tested with the change just in Gutenberg, and with the change copied over to core files)
✅ Read-only logos work correctly for non-admins
Personally I quite like the look of adding in the trash icon for the Remove button. I just left a comment on that — if we go with the icon, it could be good to add in a tooltip label.
Once Joen or others are happy with the design and give it the ✅, I think this PR will be good to land.
| function _delete_custom_logo_on_remove_site_logo() { | ||
| $theme = get_option( 'stylesheet' ); | ||
|
|
||
| // Unhook update and delete actions for custom_logo to prevent a loop of hooks. |
There was a problem hiding this comment.
Good idea adding this to avoid accidentally causing a loop of hooks.
✅ Tested that with this code in the Gutenberg plugin, by execution time has_action returns false so these actions aren't registered, however the code doesn't appear to throw any warnings or cause any issues
✅ Tested that with this code copied to the site-title.php file in public_html/wp-includes/blocks/site-logo.php the actions are registered, so this code prevents the loop
For testing I added the following and inspected by debug.log file:
$has_action_1 = has_action( "update_option_theme_mods_$theme", '_delete_site_logo_on_remove_custom_logo' );
$has_action_2 = has_action( "delete_option_theme_mods_$theme", '_delete_site_logo_on_remove_theme_mods' );
error_log( 'has actions' );
error_log( var_export( $has_action_1, true ) );
error_log( var_export( $has_action_2, true ) );
The results were:
In Gutenberg plugin:
[17-Sep-2021 01:22:46 UTC] has actions
[17-Sep-2021 01:22:46 UTC] false
[17-Sep-2021 01:22:46 UTC] false
In Core:
[17-Sep-2021 01:36:55 UTC] has actions in core!
[17-Sep-2021 01:36:55 UTC] 10
[17-Sep-2021 01:36:55 UTC] 10
There was a problem hiding this comment.
Gutenberg's build process is getting in the way here: because _delete_site_logo_on_remove_custom_logo and _delete_site_logo_on_remove_theme_mods are function names in this file, they are being automatically prefixed with gutenberg_.
This means that the unprefixed Core hooks for the site logo block are not removed, and end up running with remove_theme_mod( 'custom_logo' ), the result being that delete_option( 'site_logo' ); can be called twice. There's a check that prevents this from becoming an infinite loop, but I still think we should prevent this from happening.
You can verify this with the following steps
- Delete any existing site_logo (block) and custom_logo (Customizer) settings
- Add a custom logo by setting the logo in the Customizer
- Insert the Site Logo block in a post/page (you'll see the custom logo you set)
- Reset the logo using the button added in this PR
- If you add a log call or breakpoint to
_delete_site_logo_on_remove_custom_logoin https://github.com/WordPress/wordpress-develop/blob/0f0089391b0f92d4508b8a4df567df2052161340/src/wp-includes/blocks/site-logo.php#L135, you'll see this function is still called on the"update_option_theme_mods_$theme"hook.
Without changing the build process, we could make the relevant function names in this file different from core, and that would work around the issue... something like
function gutenberg_delete_site_logo_on_remove_custom_logo( $old_value, $value ) { ...
function gutenberg_delete_site_logo_on_remove_theme_mods() { ...
function _delete_custom_logo_on_remove_site_logo() {
$theme = get_option( 'stylesheet' );
// Unhook update and delete actions for custom_logo to prevent a loop of hooks.
// Gutenberg hooks.
remove_action( "update_option_theme_mods_$theme", 'gutenberg_delete_site_logo_on_remove_custom_logo', 10 );
remove_action( "delete_option_theme_mods_$theme", 'gutenberg_delete_site_logo_on_remove_theme_mods' );
// Core hooks.
remove_action( "update_option_theme_mods_$theme", '_delete_site_logo_on_remove_custom_logo', 10 );
remove_action( "delete_option_theme_mods_$theme", '_delete_site_logo_on_remove_theme_mods' );
// Remove the custom logo.
remove_theme_mod( 'custom_logo' );
// Restore update and delete actions.
// Gutenberg hooks.
add_action( "update_option_theme_mods_$theme", 'gutenberg_delete_site_logo_on_remove_custom_logo', 10, 2 );
add_action( "delete_option_theme_mods_$theme", 'gutenberg_delete_site_logo_on_remove_theme_mods' );
// Core hooks.
add_action( "update_option_theme_mods_$theme", '_delete_site_logo_on_remove_custom_logo', 10, 2 );
add_action( "delete_option_theme_mods_$theme", '_delete_site_logo_on_remove_theme_mods' );
}
add_action( 'delete_option_site_logo', '_delete_custom_logo_on_remove_site_logo' );It's a little strange, as those functions will become prefixed with gutenberg_gutenberg_, but in practice I don't think that matters.
Another option would be to remove _delete_site_logo_on_remove_custom_logo_on_setup_theme from this file, and the two functions it hooks in (_delete_site_logo_on_remove_custom_logo and _delete_site_logo_on_remove_theme_mods).
Because this file is included on the init hook, and _delete_site_logo_on_remove_custom_logo_on_setup_theme is hooked into setup_theme, which runs before init, these functions are never run from Gutenberg (only from Core). They seem to be here only to keep this file consistent with Core. (related issue: #33177)
There was a problem hiding this comment.
There's a check that prevents this from becoming an infinite loop, but I still think we should prevent this from happening.
That explains how I missed it 😱 Thank you for checking this!
I went with the first suggestion to rename the hooks in Gutenberg and unhook/rehook both. I'm a bit torn since I agree it's a little strange, but I think doing so actually makes the hook duplication clearer.
There was a problem hiding this comment.
That fixed the issue, thanks!
There was a problem hiding this comment.
Thanks for adding in the label text @stacimc, this is still testing nicely for me! The code change looks good, so I'm giving it the ✅
I noticed one slight design issue when switching the block editor to use "Display button labels" instead of icons. In that mode, the text size for icon-based buttons is slightly smaller than the normal text size of text-based buttons, so without a separating line the sizes are a tiny bit mismatched:
(To set this mode, in the editor go to Options > Preferences > Set Display Button Labels to True)
I imagine that's something at the component level, so we could look into that in a separate PR if it's an issue 🙂
|
In testing this PR just now, I'm still seeing the trash icon: That probably okay if folks feel strongly for that option, but I do have a small preference for starting with option 1 or 2 from this list of exporations, which just shows "Replace" and "Remove" labels side by side (the full text "Remove logo" feels redundant). |
Personally I don't have a strong preference, so happy to go back to the text button. I do like the consistency with Removing the @jasmussen What do you think? |
|
The PR has surfaced a need for some code & design love for multiple text buttons — likely an opportunity to improve how "label only" mode looks as well. I'll make a note to look at it if I get time. But if the 2nd option feels hacky and is visually offset, we should probably go with option 1 for now. |
|
Looking more closely at the screenshot (I'm unable to test in my environment currently) it appears as if the font size is bigger than that of the Image block replace button, so just to be sure, the font size should be 13px same as other buttons. That might also explain the height/offset differential in your second option. I'm out tomorrow, otherwise I'd be happy to dive in and take a look. |
Font size is 13px for both buttons 🤔 I took another look -- the issue is that there's a If we wrap both buttons in The solution I have right now is to keep the
Agreed! Nesting
|
|
Took the latest branch for a spin, and this is what I see: I think it's fine to go with this for now, thanks for the back and forth. It doesn't preclude future iterations, notably the block toolbar and how it's pieced together could use a big refactor to embrace flex and simpler markup/css — but that shouldn't block this nice little improvement from landing. |
|
I imagine there will be lots of confusion over the word "Remove" and if it relates to the "Remove block" item in the block's ellipsis menu:
Perhaps "Reset" would be a better label for this button? |
|
Good point, or "Clear". |
|
"Reset" matches with the new Tools Panel:
|
stacimc
force-pushed
the
add/remove-site-logo-from-block
branch
from
0391de2 to
50cf885
Compare
That's a great point -- I'm sold! Changed to |
|
@creativecoder I think this is almost good to go; would you mind taking a final look at the syncing of the Site Logo with the custom logo? |
|
One good reason to use "Remove" or "Remove logo" as text label instead of trash-icon is to avoid mixing it with "Remove block":
|
| function _delete_custom_logo_on_remove_site_logo() { | ||
| $theme = get_option( 'stylesheet' ); | ||
|
|
||
| // Unhook update and delete actions for custom_logo to prevent a loop of hooks. |
There was a problem hiding this comment.
Gutenberg's build process is getting in the way here: because _delete_site_logo_on_remove_custom_logo and _delete_site_logo_on_remove_theme_mods are function names in this file, they are being automatically prefixed with gutenberg_.
This means that the unprefixed Core hooks for the site logo block are not removed, and end up running with remove_theme_mod( 'custom_logo' ), the result being that delete_option( 'site_logo' ); can be called twice. There's a check that prevents this from becoming an infinite loop, but I still think we should prevent this from happening.
You can verify this with the following steps
- Delete any existing site_logo (block) and custom_logo (Customizer) settings
- Add a custom logo by setting the logo in the Customizer
- Insert the Site Logo block in a post/page (you'll see the custom logo you set)
- Reset the logo using the button added in this PR
- If you add a log call or breakpoint to
_delete_site_logo_on_remove_custom_logoin https://github.com/WordPress/wordpress-develop/blob/0f0089391b0f92d4508b8a4df567df2052161340/src/wp-includes/blocks/site-logo.php#L135, you'll see this function is still called on the"update_option_theme_mods_$theme"hook.
Without changing the build process, we could make the relevant function names in this file different from core, and that would work around the issue... something like
function gutenberg_delete_site_logo_on_remove_custom_logo( $old_value, $value ) { ...
function gutenberg_delete_site_logo_on_remove_theme_mods() { ...
function _delete_custom_logo_on_remove_site_logo() {
$theme = get_option( 'stylesheet' );
// Unhook update and delete actions for custom_logo to prevent a loop of hooks.
// Gutenberg hooks.
remove_action( "update_option_theme_mods_$theme", 'gutenberg_delete_site_logo_on_remove_custom_logo', 10 );
remove_action( "delete_option_theme_mods_$theme", 'gutenberg_delete_site_logo_on_remove_theme_mods' );
// Core hooks.
remove_action( "update_option_theme_mods_$theme", '_delete_site_logo_on_remove_custom_logo', 10 );
remove_action( "delete_option_theme_mods_$theme", '_delete_site_logo_on_remove_theme_mods' );
// Remove the custom logo.
remove_theme_mod( 'custom_logo' );
// Restore update and delete actions.
// Gutenberg hooks.
add_action( "update_option_theme_mods_$theme", 'gutenberg_delete_site_logo_on_remove_custom_logo', 10, 2 );
add_action( "delete_option_theme_mods_$theme", 'gutenberg_delete_site_logo_on_remove_theme_mods' );
// Core hooks.
add_action( "update_option_theme_mods_$theme", '_delete_site_logo_on_remove_custom_logo', 10, 2 );
add_action( "delete_option_theme_mods_$theme", '_delete_site_logo_on_remove_theme_mods' );
}
add_action( 'delete_option_site_logo', '_delete_custom_logo_on_remove_site_logo' );It's a little strange, as those functions will become prefixed with gutenberg_gutenberg_, but in practice I don't think that matters.
Another option would be to remove _delete_site_logo_on_remove_custom_logo_on_setup_theme from this file, and the two functions it hooks in (_delete_site_logo_on_remove_custom_logo and _delete_site_logo_on_remove_theme_mods).
Because this file is included on the init hook, and _delete_site_logo_on_remove_custom_logo_on_setup_theme is hooked into setup_theme, which runs before init, these functions are never run from Gutenberg (only from Core). They seem to be here only to keep this file consistent with Core. (related issue: #33177)
| function _delete_custom_logo_on_remove_site_logo() { | ||
| $theme = get_option( 'stylesheet' ); | ||
|
|
||
| // Unhook update and delete actions for custom_logo to prevent a loop of hooks. |
There was a problem hiding this comment.
That fixed the issue, thanks!
|
@jasmussen Just double checking that this one is ready to merge from a UI perspective with the new |
|
Yep let's try it! |
| @@ -132,7 +132,7 @@ function _sync_custom_logo_to_site_logo( $value ) { | |||
| * @param array $old_value Previous theme mod settings. | |||
| * @param array $value Updated theme mod settings. | |||
| */ | |||
| function _delete_site_logo_on_remove_custom_logo( $old_value, $value ) { | |||
| function _gutenberg_delete_site_logo_on_remove_custom_logo( $old_value, $value ) { | |||
There was a problem hiding this comment.
This isn't ideal because it means that, when these changes are merged into Core, that Core will have a function prefixed with _gutenberg.
| add_action( "update_option_theme_mods_$theme", '_delete_site_logo_on_remove_custom_logo', 10, 2 ); | ||
| add_action( "delete_option_theme_mods_$theme", '_delete_site_logo_on_remove_theme_mods' ); |
There was a problem hiding this comment.
_delete_site_logo_on_remove_custom_logo and _delete_site_logo_on_remove_theme_mods both won't exist when Core's copy of site-logo.php is updated to be the site-logo/index.php that we see here. This causes a fatal error.
Fatal error: Uncaught TypeError: call_user_func_array(): Argument #1 ($callback) must be a valid callback, function "_delete_site_logo_on_remove_custom_logo" not found or invalid function name
I think we should revert 0140ba0 and work around the issue with Gutenberg's build process in a different way. I'll open a PR to do this.
Closes #34796, tracked in #30406
Description
This PR adds a way to clear/remove the media from the Site Logo from within the block. I've added this as
aaRemovebutton in the block toolbar, similar toReplace, an icon in the block toolbar, but would love design feedback.Resetbutton in the block toolbar, similar toReplace, and in keeping with the terminology used by the ToolsPanel.How has this been tested?
I've attempted to test all the edge cases described in #33179 and #32919 for regressions with these new changes.
**IMPORTANT TESTING INSTRUCTIONS: ** Due to an issue raised in #33177, it's tricky to test updates to the actions (basically, the Core php file and the plugin php file for site-logo both get loaded), so you will need to either manually disable the similar code that's running in Core (
wp-includes/blocks/site-logo.php) or copy over the changes in this PR to that file as well.Simple flow:
ReplacenorResetoptions are present in the toolbar.ReplaceandResetoptions.Replaceoption still worksReset; the placeholder should render againWhen the logo is persisted:
Reset. You should see the placeholder rendered again.Custom Logo through customizer:
Resetand save the post & Site logoRead-only logos for non-admins:
ReplaceandResetbuttons should not be available. They should still be able to edit block attributes like the rounded style, width, etc.Screenshots
Updated design using 'Reset':
Read-only logo with no option to remove/replace, but access to block attrs (design has changed slightly to
Reset):Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).