Site Logo Block: update Site Logo block UI and option syncing #33179
Conversation
creativecoder
added
Backport to WP 6.9 Beta/RC
creativecoder
requested review from
TimothyBJacobs,
ajitbohra and
spacedmonkey
as code owners
|
Size Change: -136 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
| const { | ||
| siteLogoId, | ||
| canUserEdit, | ||
| url, | ||
| mediaItemData, | ||
| isRequestingMediaItem, | ||
| } = useSelect( ( select ) => { | ||
| const { canUser, getEntityRecord, getEditedEntityRecord } = select( | ||
| coreStore | ||
| ); | ||
| const siteSettings = getEditedEntityRecord( 'root', 'site' ); | ||
| const siteData = getEntityRecord( 'root', '__unstableBase' ); | ||
| const _siteLogo = siteSettings?.site_logo; | ||
| const _readOnlyLogo = siteData?.site_logo; | ||
| const _canUserEdit = canUser( 'update', 'settings' ); | ||
| const _siteLogoId = _siteLogo || _readOnlyLogo; | ||
| const mediaItem = | ||
| _siteLogoId && | ||
| select( coreStore ).getEntityRecord( 'root', 'media', _siteLogoId, { | ||
| context: 'view', | ||
| } ); | ||
| const _isRequestingMediaItem = | ||
| _siteLogoId && | ||
| select( coreStore ).isResolving( 'getEntityRecord', [ | ||
| 'root', | ||
| 'media', | ||
| _siteLogoId, | ||
| { context: 'view' }, | ||
| ] ); | ||
| return { | ||
| siteLogoId: _siteLogoId, | ||
| canUserEdit: _canUserEdit, | ||
| url: siteData?.url, | ||
| mediaItemData: mediaItem && { | ||
| url: mediaItem.source_url, | ||
| alt: mediaItem.alt_text, | ||
| }, | ||
| isRequestingMediaItem: _isRequestingMediaItem, | ||
| }; | ||
| }, [] ); |
There was a problem hiding this comment.
Most of this change is auto-formatting. The only substantial change is adding isRequestingMediaItem.
| .editor-styles-wrapper { | ||
| .site-logo_placeholder { | ||
| display: flex; | ||
| flex-direction: row; | ||
| align-items: flex-start; | ||
| border-radius: $radius-block-ui; | ||
| background-color: $white; | ||
| box-shadow: inset 0 0 0 $border-width $gray-900; | ||
| padding: $grid-unit-15; | ||
| svg { | ||
| margin-right: $grid-unit-15; | ||
| } | ||
| p { | ||
| font-family: $default-font; | ||
| font-size: $default-font-size; | ||
| margin: 0; | ||
| line-height: initial; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
By changing to the <Placeholder> component, these overrides don't seem to be needed.
| function render_block_core_site_logo( $attributes ) { | ||
| $adjust_width_height_filter = function ( $image ) use ( $attributes ) { | ||
| if ( empty( $attributes['width'] ) ) { | ||
| if ( empty( $attributes['width'] ) || empty( $image ) ) { |
There was a problem hiding this comment.
This prevents a PHP warning from the code below if the image was deleted without updating the logo setting.
|
|
||
| /** | ||
| * Updates the custom_logo theme-mod when the site_logo option gets updated. | ||
| * | ||
| * @param mixed $old_value The old option value. | ||
| * @param mixed $value The new option value. | ||
| * | ||
| * @return void | ||
| */ | ||
| function _sync_site_logo_to_custom_logo( $old_value, $value ) { | ||
| // Delete the option when the custom logo does not exist or was removed. | ||
| // This step ensures the option stays in sync. | ||
| if ( empty( $value ) ) { | ||
| remove_theme_mod( 'custom_logo' ); | ||
| } else { | ||
| remove_filter( 'pre_set_theme_mod_custom_logo', '_sync_custom_logo_to_site_logo' ); | ||
| set_theme_mod( 'custom_logo', $value ); | ||
| add_filter( 'pre_set_theme_mod_custom_logo', '_sync_custom_logo_to_site_logo' ); | ||
| } | ||
| } | ||
|
|
||
| add_action( 'update_option_site_logo', '_sync_site_logo_to_custom_logo', 10, 2 ); |
There was a problem hiding this comment.
Please see the PR description for why I'm proposing to remove this.
|
@youknowriad @aristath @ntsekouras I'd appreciate your feedback on these changes. They seem urgent enough to me to warrant backporting to WP 5.8, if possible. Also, I'm happy to break them up if that's more convenient. Also, note an issue regarding code loading for this block, as it affects testing this PR. |
| } else { | ||
| remove_filter( 'pre_set_theme_mod_custom_logo', '_sync_custom_logo_to_site_logo' ); | ||
| set_theme_mod( 'custom_logo', $value ); | ||
| add_filter( 'pre_set_theme_mod_custom_logo', '_sync_custom_logo_to_site_logo' ); |
There was a problem hiding this comment.
Note that this line has a bug in it that can delete the site_logo by calling _sync_custom_logo_to_site_logo with just the custom logo value instead of with the theme mods array. If we don't remove this code, we at least need to fix this filter.
| return $value; | ||
| } | ||
|
|
||
| add_filter( 'pre_set_theme_mod_custom_logo', '_sync_custom_logo_to_site_logo' ); |
There was a problem hiding this comment.
I'm restoring the use of the pre_set_theme_mod_custom_logo filter because it solves a couple problems that happen when switching to the update_option_theme_mods_$theme action in #32370.
- Make sure the site_logo isn't deleted in the case where a theme isn't setting a logo because it doesn't support one
- Prevent the site_logo from being deleted when switching themes.
| 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', 10, 0 ); | ||
| } | ||
| add_action( 'setup_theme', '_delete_site_logo_on_remove_custom_logo_on_setup_theme', 11 ); |
There was a problem hiding this comment.
These hooks and functions cover deleting the site_logo when a custom_logo is removed with remove_theme_mod or remove_theme_mods.
These should allow this change to pass Core phpunit tests (the theme used for the unit tests will need to declare support for custom-logo).
aristath
left a comment
aristath
left a comment
There was a problem hiding this comment.
I'm a bit torn here...
I like what this PR is doing, but at the same time it feels like it switches the logic a bit. The option becomes the primary source of truth instead of the theme-mod, and I'm not sure if that's good or bad.
So far, the standard way of adding or changing a logo on a site is not the site-logo block... it's the customizer.
Though block themes are the future, right now they are not the norm. Themes should work the way they previously were, and the site-logo block in WP 5.8 will be primarily used in block templates. The custom_logo theme-mod at this stage is the primary source of the logo, and the site_logo option is the future implementation we're trying to switch to.
But the site_logo option can't be treated as the main setting for logos until block-themes are the norm... Themes and plugins may be adding filters to the theme-mod and developers will expect everything to work the way it did before. If a classic theme has a block template, the same logo is expected to show in both modes consistently.
That's why the previous implementation was getting the theme-mod instead of the option, and that's why there was extra functionality implemented to keep the option and theme-mod in sync.
At this stage, the site-logo block is treated more like a new interface to edit existing data structures, it's not (yet) a complete switch to a new data structure because there were many backward-compatibility concerns.
I agree that it sometimes feels like overkill, but we need to be extra careful not to break possible implementations of plugins/themes 🤔
|
I agree with @aristath above. We need to make sure this doesn't break anything. Tests well, one small thing I've noticed: Media Library flashes in Screen.Recording.2021-07-05.at.15.01.32.mov |
|
@aristath Thanks for the prompt and thoughtful feedback.
I don't see that as being new in this PR. I think the option effectively became the source of truth when This does make me realize it's better to call
I 100% agree on the caution here. My primary goal with the filter changes in this PR is to prevent unexpected things from happening, mainly the logo disappearing unexpectedly or not getting updated from the Customizer. This is informed by a number of user issues I've seen with setting logos since v10.7.4 of the Gutenberg plugin was released. For what it's worth, restoring the use of the |
Thanks for testing @david-szabo97 ! That's a good catch--I was able to update the loading logic subtly and add a spinner to the empty placeholder to prevent this flash.
|
david-szabo97
left a comment
david-szabo97
left a comment
There was a problem hiding this comment.
Played around with the customizer and switching between themes. Works as expected!
|
Thank you for the reviews and testing! @youknowriad @aristath Is there another collection of backports planned from Gutenberg -> WP 5.8? Or should I open a trac ticket specifically for these changes? |
|
I'd also love to get these fixes into Gutenberg 11.0.0, if that's possible 🙂 |
The backport label is enough. It will be included in RC3.
--cc @getdave |
|
This has been cherry picked into 11.0.0 |
youknowriad
removed
the
Backport to WP 6.9 Beta/RC
Description
Updates for the Site Logo block, including some important bug fixes to prevent
custom-logo.Ensure
site_logoisn't deleted unintentionallyThe change in #32370 introduced a problem where the
site_logowould be deleted when switching to a theme without any theme mods settings in the database, because theupdate_option_theme_mods_$themehook would run with a default set of theme mods that didn't include a custom_logo, and then delete the site logo.This change restores the use of the
pre_set_theme_mod_custom_logohook, and uses a different set of hooks to make sure the site_logo is deleted as needed byremove_theme_modorremove_theme_mods. This should allow the change to pass Core unit tests.Fix loading bug when image does not exist
Previously, if the logo attachment ID points to a non-existent image, the block would perpetually show a loading state. This adds a check to ensure the loading state only shows when requesting the attachment information.
Improves the placeholder
In the editor, a placeholder is shown for the block when there's no image and before we know the user can edit the logo settings. Use the
<Placeholder>component, rather than a plain<div>, and update styles so that the UI transitions are smoother.Remove the site_logo -> custom_logo hooks
site_logo(when set) is always used for displaying the logo (withget_custom_logo()), so I'm not aware of a specific need to keep thecustom_logotheme_mod in sync. Also, the current implementation is incomplete: it needs additional hooks foradd_optionanddelete_option. Combined with having both the Core and Gutenberg versions of the block code, all of this adds considerable complexity that can make it very hard to track what's happening when updates are made.Because of this, I propose removing
_sync_site_logo_to_custom_logo, for now. If we ever have a specific use case (e.g. we need to delete thecustom_logosetting when the logo is deleted from the block), we could add back exactly what we need, at that time.How has this been tested?
remove_theme_mod( 'custom_logo' )(orwp theme mod remove custom_logovia cli) and see thatsite_logois removed and the theme no longer displays a logo. Check the same forremove_theme_mods().IMPORTANT: The filters registered on
setup_themeare difficult to test because of #33177. The_delete_site_logo_on_...functions in Gutenberg will not execute b/c thesetup_themeaction has already happened when the action is added.To test these changes you can
wp-includes/blocks/site-logo.phpin your development environment, orsetup_themehook toinitinsite-logo/index.phpfor the purposes of testing.Make sure you disable the similar code that's running in Core (
wp-includes/blocks/site-logo.php) to test this change in isolation.Types of changes
Refinements and bug fixes
Checklist:
*.native.jsfiles for terms that need renaming or removal).