Update Site Icon block to better align with rest of core

[aaronjorbin]

Description

This is a follow-up to #35892, #30406 and https://core.trac.wordpress.org/ticket/54370. Two things discovered:

1. Right now, the site icon block links to /wp-admin/customize.php?autofocus[section]=title_tagline in https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/site-logo/edit.js#L281 . As the Site Icon can now be edited on the general setting screen, that should be linked instead.
2. Everywhere else in core Site Icon is capitalized except for the string in https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/site-logo/edit.js#L334

Step-by-step reproduction instructions

1. Use WordPress

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[andrewserong]

Right now, the site icon block links to /wp-admin/customize.php?autofocus[section]=title_tagline in https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/site-logo/edit.js#L281 . As the Site Icon can now be edited on the general setting screen, that should be linked instead.

Good idea! I think it'll likely need a check for which WP core version is running, though, as Gutenberg can be running on core versions < 6.5, so I imagine we'll need to have a check in place until the required version is at least 6.5. Does anyone know the right way to check for a WP core version in Gutenberg JS?

Everywhere else in core Site Icon is capitalized except for the string in

I've put up a PR for this over in #59383

[ramonjd]

Does anyone know the right way to check for a WP core version in Gutenberg JS?

It's possible to add the WP Version to editor settings I suppose. We could have a hook with a condition similar to this one:

gutenberg/lib/compat/wordpress-6.5/fonts/fonts.php

Line 106 in 8f61b9b

if ( version_compare( $wp_version, '6.5-alpha', '<' ) ) {

The only hesitation I would have is that the version might be used too much, thereby creating maintenance burden.

cc @WordPress/gutenberg-core for opinions

[t-hamano]

We may also be able to define it as follows using the is_wp_version_compatible() function.

function gutenberg_get_block_editor_settings( $settings ) {
	// ...

	$settings['hasDashboardSiteIconSetting'] = is_wp_version_compatible( '6.5' );

	return $settings;
}
add_filter( 'block_editor_settings_all', 'gutenberg_get_block_editor_settings', 0 );

[Mamaduka]

The settings are public API; once introduced, we have to keep them around for backward compatibility even after the minimum supported version is WP 6.5.

[ramonjd]

The settings are public API; once introduced, we have to keep them around for backward compatibility even after the minimum supported version is WP 6.5.

So maybe something generic like the WP version number? Still, that could invite a lot of crud conditional code in the codebase 🤔

Maybe it's not so bad to just keep the link to the customizer

[carolinan]

Instead of linking to the settings page I think we should remove the site icon option completely from the site logo block.

[aaronjorbin]

Maybe it's not so bad to just keep the link to the customizer

That partially defeats the purpose of adding the setting to the general options page. For block themes, the customizer is a dead end. This is the one link from inside the block editor that many people will find. They can end up on that screen that is very different from the rest of their WordPress experience. As shaunandrews noted on trac "Involving the customizer as a solution is temporary"

Instead of linking to the settings page I think we should remove the site icon option completely from the site logo block.

I think that's worth considering, but to me it's a bit late in the 6.5 cycle to be making that decision. Perhaps a separate issue should be opened to discuss that?

@andrewserong Thank you for getting the first part in. What needs to be done to ensure #59383 is included in the next package sync to core?

[richtabor]

Instead of linking to the settings page I think we should remove the site icon option completely from the site logo block.

I think it's quite important to keep the site icon option in the block. Not every site uses a different icon/logo, and it'd be quite unexpected to remove functionality that's been in for a number of releases.

And we have to consider discoverability.

[andrewserong]

Thank you for getting the first part in. What needs to be done to ensure #59383 is included in the next package sync to core?

Apologies, I forgot to add the Backport to WP Beta/RC label on that one, so it didn't make it in for Beta 3. I've added the label, so it'll be picked up in the next sync.

[aaronjorbin]

A handful of ideas for solving this if the suggestion from @t-hamano isn't acceptable.

1. Have a site_icon_endpoint setting that returns the URL to use. This can do the version compare in PHP and then when pre 6.5 versions are no longer supported, it would just return the options page. This also makes it possible to more easily change this based on future redesigns of WordPress.
2. Introduce a javascript version of is_wp_version_compatible and use that to determine the link
3. Use process.env.IS_GUTENBERG_PLUGIN and have the bad link in the plugin but not in core
4. Use https://www.npmjs.com/package/patch-package in core and defer the change in this repo (I really don't like this option, but throwing it out there)

[aaronjorbin]

@WordPress/gutenberg-core Which of the above options sound best to y'all? Or do you have an alternative? I want to make sure this is completed before RC1.

[andrewserong]

One other potential idea, is to add another window.__experimental global as we have in gutenberg_enable_experiments, that is only present if core if < 6.5.0 — since we already have a convention of using these experimental values, and they're not meant to be a stable API, that might potentially be an acceptable place to hack it in for now, while avoiding adding any PHP code that would need to be in core.

I should have a little bit of time today, so I'll see if I can get a proof of concept PR up for that idea.

[andrewserong]

I have a PR up with the idea of adding a window.__experimentalUseCustomizerSiteLogoUrl global in Gutenberg (uses a similar approach to other Gutenberg experiments), to see if that might work. It's ready for review over in #59485

I might not have time to iterate on that PR, so if anyone feels strongly about taking a different approach, feel free to close that one out!

[youknowriad]

That sounds like a good approach to me. Thanks for looking @andrewserong