Editor: Display Site Icon (if one is set) in Gutenberg Fullscreen Mode #22952

jeyip · 2020-06-05T21:59:49Z

Description

This PR addresses issue #20929 Iterations on W icon in the editor header. It renders the current site icon (if one is set) instead of the default WordPress icon in the site and post editor.

Note:

@noahtallen made a great callout. This PR only handles displaying a user uploaded site icon. We may need to update the site icon from the Gutenberg editor in the future as well. Although not addressed here, a strategy for updating site icons is worthwhile to consider while reviewing this PR.
Since information about site icons wasn't originally provided to the Gutenberg editor, we exposed the site icon url as a settings field. We use register_setting to extend the WordPress wp/v2/settings API. @ockham discussed the idea of including that as a field in Core WordPress itself. We are considering opening a new issue, and I'd love to hear more feedback about this.

How has this been tested?

Set up local instance of WordPress and Gutenberg by following instructions here
Navigate to WordPress admin sidebar -> Appearance -> Customize -> Site Identity
Upload a site icon
Publish changes
If already enabled, navigate to the site editor
- Verify changes in full-screen mode
If not already enabled, navigate to WordPress admin sidebar -> Gutenberg -> Experiments
- Enable Full Site Editing
- Enable Full Site Editing Demo Templates
- Navigate to the site editor and verify changes in full-screen mode

Screenshots

With no uploaded site icon

With site icon

GIF

Types of changes

Expose site icon URL to Gutenberg editor
Display site icon in full-screen mode

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

Requirements

With default WordPress icon

Icon button disappears on screens smaller than 782px
Clicking icon button redirects user to wp-admin home page
Back button tooltip displays on hover after timed delay

With custom site icon

Icon button disappears on screens smaller than 782px
Clicking icon button redirects user to wp-admin home page
Back button tooltip displays on hover after timed delay
Icon is displayed for Contributor, Author, Editor, and Admin roles.

Browsers

gutenberg.php

noahtallen

This seems like a good approach to me for now. As we discussed earlier, the main concern is that updating the site_icon_url setting would not actually change the site_icon option under the hood. All of the site icon related code (like the favicon and get_site_icon_url pulls from that option under the hood. Unfortunately, since that option stores a media ID and not a URL, we can't use it directly for our purposes here.

packages/edit-site/src/components/header/fullscreen-mode-close/index.js

ockham · 2020-06-05T22:58:46Z

packages/edit-site/src/components/header/fullscreen-mode-close/index.js

+		return ! select( 'core/data' ).hasFinishedResolution(
+			'core',
+			'getEntityRecord',
+			[ 'root', 'site', undefined ]


Doesn't this need to match the useEntityProp args?

Great question. I'm not entirely sure if they have to be the same because I couldn't find great documentation on hasFinishedResolution, but I'll investigate and update you here.

one thought is that since 'root', 'site' translates to a single query which fetches /settings, there probably isn't a need to be more granular, since every prop is resolved in the same request. (I think)

packages/edit-site/src/components/header/fullscreen-mode-close/index.js

paaljoachim · 2020-06-05T23:26:11Z

I tried testing on gutenberg.run but was not able to because of errors in the customizer.
It is awesome that you are working on this! I will see if I can find another way to check out the PR.

packages/edit-site/src/components/header/fullscreen-mode-close/style.scss

youknowriad

Nice work here. Left some comments.

packages/edit-post/src/components/header/fullscreen-mode-close/index.js

gutenberg.php

packages/edit-post/src/components/header/fullscreen-mode-close/index.js

TimothyBJacobs

Looks good REST AP wise.

Addison-Stavlo · 2020-06-11T22:56:09Z

Testing this on my end works well with and without the icon set. This is great to see!

noahtallen · 2020-06-11T23:18:39Z

packages/edit-post/src/components/header/fullscreen-mode-close/index.js

-			postType: getPostType( getCurrentPostType() ),
-		};
-	}, [] );
+			return {


@mtias @youknowriad do you think we should limit this to just edit-site for now, or do you think it would also be good to start testing it in edit-post?

We should do it in both, but if we are not certain yet, better to keep it in edit-site. We would need to decide if it's ready for 5.5 as well (in edit-post).

We would need to decide if it's ready for 5.5 as well

Sounds good 👍 . I'm newer on the team so I'm not sure what this entails. Could you elaborate?

Also, I wanted to call out that I'm on the gardening rotation so development on this PR is going to slow down until the end of the month.

The second week of July is the beta period for WordPress 5.5. Anything that affects the post-editor needs to be in good shape by then.

@mtias I'm back from the gardening rotation, which leaves us a few weeks to test this feature if we want it in 5.5. I see two options here:

1. Test post-editor for 5.5 release

Pros:

Site icon feature released sooner

No need for a follow-up PR to bring post-editor to feature parity

Cons:

The accelerated timeline could cause us to miss issues, especially since code review on this PR isn't finished yet.

2. Test post-editor for 5.6 release

Pros:

More time to test the feature

Cons:

Have to follow up with a second PR to ensure feature parity between site editor and post editor

Also @sirreal Would you be able to send me documentation about how I'd make sure the post-editor is tested thoroughly with these changes for the 5.5 beta?

@mtias @vindl Since it sounds like there is more extensive testing needed before releasing this feature in edit-post, I'm leaning towards separating this feature into two PRs. The current PR would be for edit-site, and the follow-up would be for edit-post.

Since the work here isn't urgent, we wouldn't have to rush testing while we prep for 5.6. Let me know what you think.

Data from WordPress REST endpoints can be preloaded. We're effectively asking the server to inject a response on the initial page load that we know will always be triggered on that page. The alternative is waiting for the page to load, waiting for the scripts to executre, and then waiting for a response to a network request. Preloading the site icon means that upon initial page load, rather than a blank placeholder, the icon will be displayed almost immediately. Edit post preloading happens in core, and the endpoints we specify for preloading are extensible through the block_editor_preload_paths filter. This change preloads the endpoint that serves site icon data to edit post.

jeyip · 2020-07-07T21:04:49Z

I moved the edit post preloading into the compat.php file. @ockham or @noahtallen would you mind adding the "Backport to Core" label to this ticket? (I'm not a contributor to Gutenberg yet)

noahtallen · 2020-07-07T22:26:34Z

Done. Did we decide that we wanted this to land in the wp core release?

jeyip · 2020-07-08T01:00:05Z

If I understand correctly, I think we're past the 5.5 deadline. I'll defer to Riad. Happy to go with 5.5 or 5.6.

youknowriad · 2020-07-08T08:58:23Z

yes, unfortunately, we're definitely passed the deadline for 5.5.

noahtallen · 2020-07-08T20:34:56Z

that makes sense to me. 👍

packages/edit-post/src/components/header/fullscreen-mode-close/index.js

Copons · 2020-07-09T10:58:59Z

packages/edit-post/src/components/header/fullscreen-mode-close/index.js

-			className="edit-post-fullscreen-mode-close"
-			icon={ wordpress }
-			iconSize={ 36 }
+			className="edit-post-fullscreen-mode-close has-icon"


I might be missing some context, but is there a reason why we're adding the .has-icon class?

Good question.

Previously, we used to pass the wordpress icon (an SVG component from @wordpress/icons) via the icon prop to Button. The Button component forwards that prop to an Icon element it embeds, and sets the has-icon prop.

Since we're no longer passing the logo to Button via the icon prop (but wrap the Button around the logo), we're losing the .has-icon styling and have to add it back manually.

However, we might explore an alternative...

The Icon component seems to accept pretty much any valid component as a fallback. So we might be able to continue to pass in our custom log (or fallback) via Button's icon prop, and remove the manually added has-icon prop (which is arguably an implementation thing rather than part of the API).

I was experimenting a bit with this, and it would be possible. However, the Icon component is also passing a size prop to the element it clones, and <img /> elements don't support that. So let's keep things as they are.

ockham · 2020-07-13T17:03:26Z

Okay, I think this is ready to land! 🎉

shaunandrews · 2020-07-16T19:25:15Z

I'm not sure what's going on, but my local install now shows a broken image instead of the WordPress logo:

I don't think its related to this PR (I think my local install is busted in some other way), but it makes me think there should be a better fallback if the image doesn't exist, or if its been deleted from the library.

jeyip · 2020-07-16T20:08:47Z

Thanks for letting us know Shaun.

The code, as it exists today, will use the WordPress Logo as the default icon to render if a user hasn't uploaded a custom one.

better fallback if the image doesn't exist, or if its been deleted from the library

I didn't realize that the default WordPress logo could be removed. Maybe the fallback should actually be to render nothing at all? Let me know what you think @shaunandrews

shaunandrews · 2020-07-16T20:46:55Z

I didn't realize that the default WordPress logo could be removed

Sorry, I think there's a misunderstanding. I had a Site Icon setup previously, but at some point had deleted the image from my library. When this PR was merged, I noticed that the editor tried displaying my Site Icon, but the file no longer exists — hence the broken image in my previous comment.

youknowriad · 2021-03-10T07:02:22Z

lib/compat.php

+ * @return array Modified path list to preload.
+ */
+function gutenberg_preload_edit_post( $preload_paths ) {
+	$additional_paths = array( '/?context=edit' );


This ticket has been rejected on Core, should we remove that code from here?

Thanks for the heads up Riad 👍 This is on my radar -- will respond when I get the chance tomorrow.

It's been quite a while since I've looked at this code, but yeah removing it makes sense to me, especially if, according to Timothy, the index endpoint doesn't even support context query params.

I'll spin up a draft to remove this code soon.

Here's an attempt at making the entity request urls more specific to each entity, and prevents the need for preloading /?context=edit: #30482

The `site_icon_url` index was supposed to ship with WordPress 5.6, but was [WordPress/gutenberg#22952 never backported to core]. This commit backports the original PR from Gutenberg repository: * [WordPress/gutenberg#42957 #42957: REST API: Add the missing 'site_icon_url' to the index] Follow-up to [52080]. Props Mamaduka, bernhard-reiter, TimothyBlynJacobs. See #56467. Built from https://develop.svn.wordpress.org/trunk@54083 git-svn-id: http://core.svn.wordpress.org/trunk@53642 1a063a9b-81f0-0310-95a4-ce76da25c4cd

The `site_icon_url` index was supposed to ship with WordPress 5.6, but was [WordPress/gutenberg#22952 never backported to core]. This commit backports the original PR from Gutenberg repository: * [WordPress/gutenberg#42957 #42957: REST API: Add the missing 'site_icon_url' to the index] Follow-up to [52080]. Props Mamaduka, bernhard-reiter, TimothyBlynJacobs. See #56467. Built from https://develop.svn.wordpress.org/trunk@54083 git-svn-id: https://core.svn.wordpress.org/trunk@53642 1a063a9b-81f0-0310-95a4-ce76da25c4cd

The `site_icon_url` index was supposed to ship with WordPress 5.6, but was [WordPress/gutenberg#22952 never backported to core]. This commit backports the original PR from Gutenberg repository: * [WordPress/gutenberg#42957 #42957: REST API: Add the missing 'site_icon_url' to the index] Follow-up to [52080]. Props Mamaduka, bernhard-reiter, TimothyBlynJacobs. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54083 602fd350-edb4-49c9-b593-d223f7449a82

The `site_icon_url` index was supposed to ship with WordPress 5.6, but was [WordPress/gutenberg#22952 never backported to core]. This commit backports the original PR from Gutenberg repository: * [WordPress/gutenberg#42957 #42957: REST API: Add the missing 'site_icon_url' to the index] Follow-up to [52080]. Props Mamaduka, bernhard-reiter, TimothyBlynJacobs. See #56467. Built from https://develop.svn.wordpress.org/trunk@54083

The `site_icon_url` index was supposed to ship with WordPress 5.6, but was [WordPress/gutenberg#22952 never backported to core]. This commit backports the original PR from Gutenberg repository: * [WordPress/gutenberg#42957 #42957: REST API: Add the missing 'site_icon_url' to the index] Follow-up to [52080]. Props Mamaduka, bernhard-reiter, TimothyBlynJacobs. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54083 602fd350-edb4-49c9-b593-d223f7449a82

noahtallen requested review from vindl, ockham, Addison-Stavlo, noahtallen and epiqueras June 5, 2020 22:09

noahtallen assigned noahtallen and jeyip and unassigned noahtallen Jun 5, 2020

noahtallen added General Interface Parts of the UI which don't fall neatly under other labels. [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. labels Jun 5, 2020

noahtallen reviewed Jun 5, 2020

View reviewed changes

gutenberg.php Outdated Show resolved Hide resolved

jeyip requested a review from youknowriad as a code owner June 5, 2020 22:29

noahtallen reviewed Jun 5, 2020

View reviewed changes

ockham reviewed Jun 5, 2020

View reviewed changes

packages/edit-site/src/components/header/fullscreen-mode-close/index.js Outdated Show resolved Hide resolved

ockham reviewed Jun 5, 2020

View reviewed changes

packages/edit-site/src/components/header/fullscreen-mode-close/index.js Outdated Show resolved Hide resolved

vindl requested a review from mtias June 6, 2020 01:10

noahtallen reviewed Jun 8, 2020

View reviewed changes

packages/edit-site/src/components/header/fullscreen-mode-close/style.scss Outdated Show resolved Hide resolved

youknowriad reviewed Jun 9, 2020

View reviewed changes

packages/edit-post/src/components/header/fullscreen-mode-close/index.js Outdated Show resolved Hide resolved

gutenberg.php Outdated Show resolved Hide resolved

packages/edit-post/src/components/header/fullscreen-mode-close/index.js Show resolved Hide resolved

jeyip requested a review from nerrad as a code owner June 11, 2020 00:29

jeyip requested review from noahtallen, TimothyBJacobs, youknowriad and ockham June 11, 2020 00:38

jeyip changed the title ~~Display Site Icon if one is set in Gutenberg Fullscreen Mode (In Progress)~~ Display Site Icon if one is set in Gutenberg Fullscreen Mode Jun 11, 2020

TimothyBJacobs approved these changes Jun 11, 2020

View reviewed changes

noahtallen reviewed Jun 11, 2020

View reviewed changes

jeyip added 3 commits July 7, 2020 13:46

Translate alt text for site icon in edit site

af978eb

Add core trac ticket annotation

c0b49d2

noahtallen added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 7, 2020

youknowriad removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 8, 2020

Copons reviewed Jul 9, 2020

View reviewed changes

packages/edit-post/src/components/header/fullscreen-mode-close/index.js Show resolved Hide resolved

Copons reviewed Jul 9, 2020

View reviewed changes

This was referenced Jul 12, 2020

Nav sidebar: display a home icon beside the front page Automattic/wp-calypso#43976

Closed

Nav sidebar: the sidebar icon should transition to the site icon Automattic/wp-calypso#43971

Closed

ockham merged commit 73f3a1d into WordPress:master Jul 13, 2020

github-actions bot added this to the Gutenberg 8.6 milestone Jul 13, 2020

jeyip deleted the add/site-icon-to-editor branch July 14, 2020 18:00

youknowriad reviewed Mar 10, 2021

View reviewed changes

creativecoder mentioned this pull request Apr 3, 2021

Core Data: specify context=edit only when needed #30482

Merged

7 tasks

Mamaduka mentioned this pull request Aug 23, 2022

REST API: Add the missing 'site_icon_url' to the index #42957

Merged

Mamaduka mentioned this pull request Aug 31, 2022

REST API: Add the missing 'site_icon_url' to the index WordPress/wordpress-develop#3157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor: Display Site Icon (if one is set) in Gutenberg Fullscreen Mode #22952

Editor: Display Site Icon (if one is set) in Gutenberg Fullscreen Mode #22952

jeyip commented Jun 5, 2020 •

edited

Loading

noahtallen left a comment

ockham Jun 5, 2020

jeyip Jun 5, 2020 •

edited

Loading

noahtallen Jun 5, 2020

paaljoachim commented Jun 5, 2020

youknowriad left a comment

TimothyBJacobs left a comment

Addison-Stavlo commented Jun 11, 2020

noahtallen Jun 11, 2020

mtias Jun 12, 2020

jeyip Jun 17, 2020

mtias Jun 18, 2020

jeyip Jun 29, 2020 •

edited

Loading

jeyip Jul 1, 2020

jeyip commented Jul 7, 2020 •

edited

Loading

noahtallen commented Jul 7, 2020

jeyip commented Jul 8, 2020 •

edited

Loading

youknowriad commented Jul 8, 2020

noahtallen commented Jul 8, 2020

Copons Jul 9, 2020

ockham Jul 13, 2020

ockham Jul 13, 2020

ockham Jul 13, 2020

ockham commented Jul 13, 2020

shaunandrews commented Jul 16, 2020

jeyip commented Jul 16, 2020 •

edited

Loading

shaunandrews commented Jul 16, 2020

youknowriad Mar 10, 2021

jeyip Mar 17, 2021

jeyip Mar 18, 2021 •

edited

Loading

creativecoder Apr 3, 2021

Editor: Display Site Icon (if one is set) in Gutenberg Fullscreen Mode #22952

Editor: Display Site Icon (if one is set) in Gutenberg Fullscreen Mode #22952

Conversation

jeyip commented Jun 5, 2020 • edited Loading

Description

How has this been tested?

Screenshots

With no uploaded site icon

With site icon

GIF

Types of changes

Checklist:

Requirements

Browsers

noahtallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeyip Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paaljoachim commented Jun 5, 2020

youknowriad left a comment

Choose a reason for hiding this comment

TimothyBJacobs left a comment

Choose a reason for hiding this comment

Addison-Stavlo commented Jun 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeyip Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

1. Test post-editor for 5.5 release

2. Test post-editor for 5.6 release

Choose a reason for hiding this comment

jeyip commented Jul 7, 2020 • edited Loading

noahtallen commented Jul 7, 2020

jeyip commented Jul 8, 2020 • edited Loading

youknowriad commented Jul 8, 2020

noahtallen commented Jul 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham commented Jul 13, 2020

shaunandrews commented Jul 16, 2020

jeyip commented Jul 16, 2020 • edited Loading

shaunandrews commented Jul 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeyip Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeyip commented Jun 5, 2020 •

edited

Loading

jeyip Jun 5, 2020 •

edited

Loading

jeyip Jun 29, 2020 •

edited

Loading

jeyip commented Jul 7, 2020 •

edited

Loading

jeyip commented Jul 8, 2020 •

edited

Loading

jeyip commented Jul 16, 2020 •

edited

Loading

jeyip Mar 18, 2021 •

edited

Loading