Skip to content

Site Logo: Update url for site icon settings with fallback for WP core versions earlier than 6.5 #59485

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
getdave merged 2 commits into from
Mar 1, 2024
Merged

Site Logo: Update url for site icon settings with fallback for WP core versions earlier than 6.5 #59485

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
18cad0b
Site Logo: Update url for site icon settings with fallback for WP cor…
andrewserong Mar 1, 2024
b8d9ae2
Move PHP changes to compat.php file
andrewserong Mar 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/experimental/editor-settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ function gutenberg_enable_experiments() {
if ( gutenberg_is_experiment_enabled( 'gutenberg-no-tinymce' ) ) {
wp_add_inline_script( 'wp-block-library', 'window.__experimentalDisableTinymce = true', 'before' );
}

// Support the previous location for the Site Icon settings. To be removed
// when the required WP core version for Gutenberg is >= 6.5.0.
if ( ! is_wp_version_compatible( '6.5' ) ) {
wp_add_inline_script( 'wp-block-editor', 'window.__experimentalUseCustomizerSiteLogoUrl = true', 'before' );
Copy link
Member

@ramonjd ramonjd Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this could have a better home in compat/6.5?

It's related to compatibility, and also it would be deleted as a matter of course when Gutenberg supports 6.5 and above.

Copy link
Contributor

@tellthemachines tellthemachines Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a good point, stuff in lib/experimental doesn't get audited for removal when we update the lowest supported WP.

Copy link
Contributor Author

@andrewserong andrewserong Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this could have a better home in compat/6.5?

Good point. My main thinking in keeping it under lib/experimental was that compat/6.5 typically houses things that are intended to be backported, and it's probably a good thing for all the window.__experimental* globals to be co-located.

That said, I'm happy to move it around. Did you have a particular file in mind? It looks like most of the current ones are there to match files in wordpress-develop 🤔

Copy link
Contributor Author

@andrewserong andrewserong Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait, we have a compat/wordpress-6.5/compat.php file — I reckon that might be a good home, I can move it there 👍

Copy link
Contributor

@tellthemachines tellthemachines Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ideally it would be a new compat/wordpress-6.5/editor-settings.php file, because none of the existing files are related in any way 😅

I see compat folders as the place we put things we'll want to remove when we no longer support that version. It's not always stuff that's to be/has been backported, but anything that is version-specific. Really it's mostly for the sake of easier cleanup later!

Copy link
Member

@ramonjd ramonjd Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay to add a new file, e.g., lib/compat/wordpress-6.4/theme-previews.php

Also maybe with a comment to say that it doesn't need backporting?

Copy link
Contributor Author

@andrewserong andrewserong Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compat.php already has this heading * WordPress 6.5 compatibility functions., so I think that might be a good home for now.

Copy link
Contributor Author

@andrewserong andrewserong Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the discussion, folks! I've moved the location over in b8d9ae2

Let me know if you'd like any further changes! 🙂

Copy link
Member

@ramonjd ramonjd Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retested. LGTM

Copy link
Contributor

@tellthemachines tellthemachines Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

}
}

add_action( 'admin_init', 'gutenberg_enable_experiments' );
Expand Down
13 changes: 9 additions & 4 deletions packages/block-library/src/site-logo/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,14 @@ const SiteLogo = ( {
</ResizableBox>
);

// Support the previous location for the Site Icon settings. To be removed
// when the required WP core version for Gutenberg is >= 6.5.0.
const shouldUseNewUrl = ! window?.__experimentalUseCustomizerSiteLogoUrl;

const siteIconSettingsUrl = shouldUseNewUrl
? siteUrl + '/wp-admin/options-general.php'
Copy link
Member

@ramonjd ramonjd Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shame there's no id we can use as a hash value. The other options have them and are used in core, e.g., #home in esc_url( admin_url( 'options-general.php' ) . '#home' ).

wp-admin/options-general.php#choose-from-library-button is possible, but it's not great.

Which reminds me that Gutenberg should probably be using admin_url() or get_admin_url() for such links.

Copy link
Contributor Author

@andrewserong andrewserong Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which reminds me that Gutenberg should probably be using admin_url() or get_admin_url() for such links.

Great point — that could be a good thing to look into for 6.6, since folks should be able to filter admin urls and have it propagate to links in Gutenberg 👍

: siteUrl + '/wp-admin/customize.php?autofocus[section]=title_tagline';

const syncSiteIconHelpText = createInterpolateElement(
__(
'Site Icons are what you see in browser tabs, bookmark bars, and within the WordPress mobile apps. To use a custom icon that is different from your site logo, use the <a>Site Icon settings</a>.'
Expand All @@ -276,10 +284,7 @@ const SiteLogo = ( {
a: (
// eslint-disable-next-line jsx-a11y/anchor-has-content
<a
href={
siteUrl +
'/wp-admin/customize.php?autofocus[section]=title_tagline'
}
href={ siteIconSettingsUrl }
target="_blank"
rel="noopener noreferrer"
/>
Expand Down