Skip to content
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.

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

Site Editor: site icon not displayed when set #51482

Closed
vindl opened this issue Mar 29, 2021 · 11 comments
Closed

Site Editor: site icon not displayed when set #51482

vindl opened this issue Mar 29, 2021 · 11 comments
Assignees
Labels
[Closed] Fixed Issues explicitly fixed with PRs. Do not use for no longer reproducible issues. [Goal] Full Site Editing Triaged To be used when issues have been triaged. [Type] Bug

Comments

@vindl
Copy link
Member

vindl commented Mar 29, 2021

If you set the site icon in core it will replace the default WordPress button in the browsing sidebar toggle. The same thing should happen in WP.com integration but it's not working as expected currently.

Note 1: It's not possible to set site the icon in core through UI with FSE is enabled at the moment, since we hide the Customizer menu item. You can still reach it directly by appending wp-admin/customize.php to your site's URL.

Screenshot 2021-03-29 at 09 01 36

Note 2: It's possible to do this on WP.com through Calypso's settings:

Screenshot 2021-03-29 at 09 26 29

@david-szabo97
Copy link
Contributor

david-szabo97 commented Apr 1, 2021

Here are my findings:

Site logo and site title are fetched from the index endpoint. In the case of a typical Gutenberg development environment that is http://localhost:8888/index.php?rest_route=/.
image

On WP.com this endpoint should live at https://public-api.wordpress.com/wp/v2/sites/_SITE_ID_/. But this endpoint is totally different from the WordPress index endpoint.
image

Related entity is defined here
https://github.com/WordPress/gutenberg/blob/3a9dc1acc4be31f81176cacb01849b9788576f05/packages/core-data/src/entities.js#L21-L26

It uses an empty string as baseUrl which means it will call the index endpoint.

Any idea how we could access the index endpoint in this case?

@david-szabo97
Copy link
Contributor

A quick hack that leverages that the endpoint is preloaded: D59634-code

In this case it won't fire a request but load the preloaded endpoint.
https://github.com/WordPress/gutenberg/blob/14f54442424f1490c74b983bc17c7b9c4968287c/lib/full-site-editing/edit-site-page.php#L112

@david-szabo97
Copy link
Contributor

Looking forward to any thoughts @mattwiebe @Addison-Stavlo @jeyip

@vindl
Copy link
Member Author

vindl commented Apr 1, 2021

cc @creativecoder since he is working on a similar issue in #51396.

@creativecoder
Copy link
Contributor

To solve this issue in the short term for WP.com, I think something like D59634-code is what we should go with for now. It addresses the troubles I was having resolving Simple vs. Atomic API calls with #51477, so that's helpful!

Long term, I'm not convinced that the site icon url belongs at the root endpoint. It seems like it might belong in settings? But there's a lot of issues to work out--for example, the settings endpoint is only accessible by admin users for the site, currently, and anyone who can load the editor should be able to fetch the site icon url.

@creativecoder
Copy link
Contributor

Just found this trac ticket and this comment as well, further confirming the "hacky-ness" of putting the site icon in the root endpoint and depending on context=edit for it to load.

It seems like we should start working on a long term solution for this sooner rather than later.

@creativecoder
Copy link
Contributor

I dug into this a little, and here's a Gutenberg PR that removes the need for transforming / into /?context=edit, which should fix this issue with the site icon: WordPress/gutenberg#30482

@david-szabo97
Copy link
Contributor

I dug into this a little, and here's a Gutenberg PR that removes the need for transforming / into /?context=edit, which should fix this issue with the site icon: WordPress/gutenberg#30482

Are you sure that's going to fix it? 🤔 On WP.com it's a proxied request issue and it is not related to the context query parameter. The / endpoint is not accessible at all on WP.com which causes this issue. We should move these settings to a totally separate endpoint. (Just like settings endpoint)

@creativecoder
Copy link
Contributor

Are you sure that's going to fix it? 🤔 On WP.com it's a proxied request issue and it is not related to the context query parameter.

@david-szabo97 You're right, that Gutenberg change won't fix it (even though I had convinced myself it would 🤦‍♂️)

However, I think I did find a solution! After an epic xdebug session with the wpcom API, I discovered that the root endpoint is available for Simple sites, in the form of https://public-api.wordpress.com/wp-json/?rest_route=/sites/{site_id}/. Figuring that out, I put together a diff that gets the site icon url where we need it for Simple sites so the icon can be displayed in the editor.

D59822-code should resolve this issue.

@ciampo ciampo added the [Status] Fix Inbound Use when a fix is in unreleased component label Apr 7, 2021
@ciampo
Copy link
Contributor

ciampo commented Apr 7, 2021

With #51477 merged:

  • the bug should be fixed for Atomic sites that update to latest ETK version
  • the bug will be fixed for Simple sites once D59822-code gets approved and deployed

@davipontesblog davipontesblog added [Closed] Fixed Issues explicitly fixed with PRs. Do not use for no longer reproducible issues. and removed [Status] Fix Inbound Use when a fix is in unreleased component labels Apr 14, 2021
@davipontesblog
Copy link
Contributor

This is now working on simple sites and atomic sites.

@davipontesblog davipontesblog added the Triaged To be used when issues have been triaged. label Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Closed] Fixed Issues explicitly fixed with PRs. Do not use for no longer reproducible issues. [Goal] Full Site Editing Triaged To be used when issues have been triaged. [Type] Bug
Projects
None yet
Development

No branches or pull requests

5 participants