Use custom_logo theme-mod instead of site_logo setting for the site-logo block
#31994
Conversation
aristath
added
[Block] Site Logo
|
Nice, thank you for opening this PR. It looks like an interesting alternative as we always operate on a single setting specific to the current theme 👍🏻 One thing I'm contemplating now is whether we could flatten this new setting included in REST API. We add handlers for set and get so in theory we could expose it as a top-level setting and handle the complex logic in PHP. What do you think? We could also move updates to REST API endpoint to |
Done. It's a lot cleaner now 👍 |
Good point, there's no reason for these to be in the block itself 👍 Done. |
aristath
requested review from
ajitbohra,
spacedmonkey and
TimothyBJacobs
as code owners
|
I think the main disadvantage to this approach is that the |
|
Size Change: +67 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
Yes. That is the way it was always working in WordPress, logos are theme-specific because themes can (and do) change the way logos are displayed, therefore the logo used for one theme is not the same that should be used for all themes 👍 |
I see the logic of that when the display of the logo is dependent on the theme's templates, as it has been with the But with a Site Logo block that's placed within the content or in an editor based template, the logo disappearing from the block on theme switch seems very disruptive to me. I think we have an opportunity to improve the experience when switching themes by making the block setting independent from the theme. |
|
Are we just reusing the settings endpoint because it is an existing endpoint we can attach on to? Am I understanding correctly that it will now always be stored in a theme mod, not in an option? |
|
We're reusing the settings endpoint because theme-mods are settings ( |
|
Ok, I'm a pretty firm -1 on this then. We shouldn't be using the settings endpoint just because it allows us to use an endpoint. The fact that theme mods are eventually stored as settings isn't really relevant since we aren't exposing or utilizing the theme mod option. The themes endpoint should instead allow for updating theme mods that have been registered. For instance thru a |
aristath
force-pushed
the
try/site-logo-use-theme-mod
branch
from
e1b66a8 to
1880dd1
Compare
Thank you for the feedback @TimothyBJacobs. That's what we were initially doing, but then moved the As for the alternatives, what we previously had was exposing a new |
|
It seems to me that in the rush to integrate with Core (which I understand!), we're sacrificing the user experience of this block. Here's what happens when you switch themes. I understand this has been normalized by how custom-logo works now in the Customizer, but in the context of blocks and the editor, it seems quite broken to me.
(Also the block is broken on trunk right now if you don't have a logo set, but that's a relatively minor fix) |
|
I created a separate issue so we can discuss this more: #32065 |
Description
In the previous iteration we were adding a
site_logosetting, and then we were trying to sync the setting with the pre-existingcustom_logotheme-mod. This PR removes thesite_logosetting, and uses thecustom_logotheme-mod to get and set the site-logo.To do that,
theme_mods.custom_logoandstylesheetwere added to the Settings REST API, and then the block's edit JS was tweaked accordingly.How has this been tested?
Tested by adding a site-logo block, setting and changing the logo.
Then went in the database and confirmed that values were properly formatted in all cases.
Checklist:
*.native.jsfiles for terms that need renaming or removal).