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 Logo: Add option to set site icon from Site Logo block #35892

Merged
merged 19 commits into from
Dec 21, 2021

Conversation

stacimc
Copy link
Contributor

@stacimc stacimc commented Oct 22, 2021

Related to: #30406

Description

Adds an option in the Inspector Controls of the Site Logo block to sync the site logo image to the Site Icon. When the toggle is switched on, any updates persisted to the Site Logo image are also made to the Icon.

When you set a new Site Logo for the first time (ie add a new Logo block and upload an image when it was previously undefined), if there is no existing site icon this toggle will be automatically turned on and the logo will be synced to the icon. When the user saves the post, they Save prompt makes it clear that they are about to persist changes to the logo and/or icon.

Any unsaved changes to the Logo & Icon are discarded when you remove the last Site Logo block from a post (see testing instructions).

Notes

  • The help text for the toggle ought to link you to a settings page where you can upload a different image or reset your site icon. Right now it links to the Customizer's Site Identity section.
    • On blocks themes, the Customizer link is removed from wp-admin, but it can still be linked to directly. There is an issue open for adding a new flow for updating site icons through the site editor (Site Editor: not possible to change site icon #29126). When this is resolved, this link ought to be updated.
  • The link to the Customizer currently opens in a new tab. This is to prevent the blocking alert to Save Changes/losing changes.

How has this been tested?

Test cases for an authorized user:

Test initial value of the toggle:

  • Make sure you have no Site Logo or Site Icon set on your site. Open a post and add a Site Logo block. Upload an image and verify in the Inspector controls that the toggle is automatically turned on. Save the post and verify that you are asked if you want to save changes to both the logo and the icon. Verify the changes save correctly.
  • Remove your Site Logo but make sure you have a Site Icon set. Open a post and insert a Site Logo block. Upload an image and verify that the toggle is not automatically turned on. Save the post and verify that you are asked if you want to save changes to the logo but NOT the icon.

Test updating the Site Logo:

  • Replace the Site Logo with a different image and save the post. You should again be prompted to save both the icon and the logo. Verify the changes persist.
  • Remove the Site Logo with the Reset option from the Replace menu in the block toolbar. You should again be prompted to save both the icon and the logo. Verify changes persist after saving (icon and logo should both be removed).
  • Replace the Site Logo with a new image. Toggle the option to sync off. Save the post. You should only be prompted to save the Logo. Verify that the new Logo is added but the icon is still empty.

Test persistence of Toggle state:

  • Toggle the sync option on and upload a new image. Save all entities. Then toggle the option off. You should be able to save the post. When you refresh, the toggle should still be turned off even though both logo and icon are the same image.

Test staying synced with changes via the Customizer:

  • Toggle the sync option on and save with a new image. Verify both icon and logo are updated. Then go to the customizer and update the site icon through the Site Identity section. Open the post in the block editor again and verify that the toggle is now OFF.
  • Toggle the sync option and upload a new image. Click 'save' and verify you see prompts to save both icon and logo, but do not actually save. Go to the customizer and upload/publish a new icon. Return to the block editor tab and now hit Save, without refreshing first. Since the toggle is still enabled, the changes to the icon here should override what you saved in the customizer.

Test discarding unsaved changes:

  • Insert a Site Logo block into a new post. Update the image and make sure the toggle is on. Verify you see the option to save both Logo & Icon in the pre-save menu, but do not save yet.
  • Insert at least one more Site Logo block. Note that it displays with the updated image.
  • Hover over the 'Default' and 'Rounded' styles in the Controls, and then move the cursor away so the preview renders and then disappears. This is to test that unmounting the Site Logo block in the preview does not cause unsaved changes to be discarded. Verify that the Logos in your post still show your unsaved change.
  • Open the large block inserter panel from the + in the toolbar. Scroll down to Site Logo and hover over it to make the preview appear, then move the cursor away so it disappears. This is to test that unmounting the Site Logo block in this preview also does not cause unsaved changes to be discarded. Again, the Logos in your post should be unaffected.
  • Remove the first Logo block. Because there's still a Logo block on the page, unsaved changes should not be discarded yet.
  • Remove the second/final Logo block. Your unsaved changes should be discarded: you should not see Logo or Icon in the pre-save menu, and if you insert a new Site Logo block it should display the published logo. Save the post and verify that the changes were not made.
  • Repeat the above tests in the site editor; make sure you include multiple Site Logo blocks in different templates. (For example, add one to the Header and one to the Footer; make an unsaved change; remove one block and verify the change is not discarded; remove the second block and verify that it is).

Tests as an unauthorized user (for example with the Author role):

  • Insert a Site Logo block in a new post. Verify that you don't see the toggle in the Inspector Controls.

Screenshots

Controls (updated to show the new copy):
Screen Shot 2021-12-07 at 1 44 33 PM

Save prompt:
Screen Shot 2021-11-01 at 11 47 20 AM

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • 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 (please manually search all *.native.js files for terms that need renaming or removal).

@stacimc stacimc added the [Status] In Progress Tracking issues with work in progress label Oct 22, 2021
@stacimc stacimc self-assigned this Oct 22, 2021
@github-actions
Copy link

github-actions bot commented Oct 22, 2021

Size Change: +505 B (0%)

Total Size: 1.13 MB

Filename Size Change
build/block-library/index.min.js 165 kB +496 B (0%)
build/editor/index.min.js 37.9 kB +9 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 960 B
build/admin-manifest/index.min.js 1.1 kB
build/annotations/index.min.js 2.75 kB
build/api-fetch/index.min.js 2.21 kB
build/autop/index.min.js 2.12 kB
build/blob/index.min.js 459 B
build/block-directory/index.min.js 6.28 kB
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-editor/index.min.js 140 kB
build/block-editor/style-rtl.css 14.6 kB
build/block-editor/style.css 14.6 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 58 B
build/block-library/blocks/audio/editor.css 58 B
build/block-library/blocks/audio/style-rtl.css 111 B
build/block-library/blocks/audio/style.css 111 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 470 B
build/block-library/blocks/button/editor.css 470 B
build/block-library/blocks/button/style-rtl.css 560 B
build/block-library/blocks/button/style.css 560 B
build/block-library/blocks/buttons/editor-rtl.css 291 B
build/block-library/blocks/buttons/editor.css 291 B
build/block-library/blocks/buttons/style-rtl.css 275 B
build/block-library/blocks/buttons/style.css 275 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 79 B
build/block-library/blocks/categories/style.css 79 B
build/block-library/blocks/code/style-rtl.css 90 B
build/block-library/blocks/code/style.css 90 B
build/block-library/blocks/code/theme-rtl.css 134 B
build/block-library/blocks/code/theme.css 134 B
build/block-library/blocks/columns/editor-rtl.css 210 B
build/block-library/blocks/columns/editor.css 208 B
build/block-library/blocks/columns/style-rtl.css 502 B
build/block-library/blocks/columns/style.css 501 B
build/block-library/blocks/comment-template/style-rtl.css 127 B
build/block-library/blocks/comment-template/style.css 127 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 242 B
build/block-library/blocks/comments-pagination/style.css 237 B
build/block-library/blocks/cover/editor-rtl.css 546 B
build/block-library/blocks/cover/editor.css 547 B
build/block-library/blocks/cover/style-rtl.css 1.22 kB
build/block-library/blocks/cover/style.css 1.22 kB
build/block-library/blocks/embed/editor-rtl.css 488 B
build/block-library/blocks/embed/editor.css 488 B
build/block-library/blocks/embed/style-rtl.css 417 B
build/block-library/blocks/embed/style.css 417 B
build/block-library/blocks/embed/theme-rtl.css 124 B
build/block-library/blocks/embed/theme.css 124 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 255 B
build/block-library/blocks/file/style.css 255 B
build/block-library/blocks/file/view.min.js 322 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 966 B
build/block-library/blocks/gallery/editor.css 970 B
build/block-library/blocks/gallery/style-rtl.css 1.63 kB
build/block-library/blocks/gallery/style.css 1.62 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 159 B
build/block-library/blocks/group/editor.css 159 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 114 B
build/block-library/blocks/heading/style.css 114 B
build/block-library/blocks/html/editor-rtl.css 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 810 B
build/block-library/blocks/image/editor.css 809 B
build/block-library/blocks/image/style-rtl.css 507 B
build/block-library/blocks/image/style.css 511 B
build/block-library/blocks/image/theme-rtl.css 124 B
build/block-library/blocks/image/theme.css 124 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 137 B
build/block-library/blocks/latest-posts/editor.css 137 B
build/block-library/blocks/latest-posts/style-rtl.css 528 B
build/block-library/blocks/latest-posts/style.css 527 B
build/block-library/blocks/list/style-rtl.css 94 B
build/block-library/blocks/list/style.css 94 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 493 B
build/block-library/blocks/media-text/style.css 490 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 649 B
build/block-library/blocks/navigation-link/editor.css 650 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B
build/block-library/blocks/navigation-link/style.css 94 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation-submenu/view.min.js 343 B
build/block-library/blocks/navigation/editor-rtl.css 1.91 kB
build/block-library/blocks/navigation/editor.css 1.92 kB
build/block-library/blocks/navigation/style-rtl.css 1.8 kB
build/block-library/blocks/navigation/style.css 1.8 kB
build/block-library/blocks/navigation/view.min.js 2.82 kB
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 377 B
build/block-library/blocks/page-list/editor.css 377 B
build/block-library/blocks/page-list/style-rtl.css 172 B
build/block-library/blocks/page-list/style.css 172 B
build/block-library/blocks/paragraph/editor-rtl.css 157 B
build/block-library/blocks/paragraph/editor.css 157 B
build/block-library/blocks/paragraph/style-rtl.css 273 B
build/block-library/blocks/paragraph/style.css 273 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/style-rtl.css 446 B
build/block-library/blocks/post-comments-form/style.css 446 B
build/block-library/blocks/post-comments/style-rtl.css 507 B
build/block-library/blocks/post-comments/style.css 507 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 721 B
build/block-library/blocks/post-featured-image/editor.css 721 B
build/block-library/blocks/post-featured-image/style-rtl.css 153 B
build/block-library/blocks/post-featured-image/style.css 153 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 391 B
build/block-library/blocks/post-template/style.css 392 B
build/block-library/blocks/post-terms/style-rtl.css 73 B
build/block-library/blocks/post-terms/style.css 73 B
build/block-library/blocks/post-title/style-rtl.css 80 B
build/block-library/blocks/post-title/style.css 80 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 198 B
build/block-library/blocks/pullquote/editor.css 198 B
build/block-library/blocks/pullquote/style-rtl.css 389 B
build/block-library/blocks/pullquote/style.css 388 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 273 B
build/block-library/blocks/query-pagination/style.css 269 B
build/block-library/blocks/query/editor-rtl.css 131 B
build/block-library/blocks/query/editor.css 132 B
build/block-library/blocks/quote/style-rtl.css 187 B
build/block-library/blocks/quote/style.css 187 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 397 B
build/block-library/blocks/search/style.css 398 B
build/block-library/blocks/search/theme-rtl.css 64 B
build/block-library/blocks/search/theme.css 64 B
build/block-library/blocks/separator/editor-rtl.css 99 B
build/block-library/blocks/separator/editor.css 99 B
build/block-library/blocks/separator/style-rtl.css 245 B
build/block-library/blocks/separator/style.css 245 B
build/block-library/blocks/separator/theme-rtl.css 172 B
build/block-library/blocks/separator/theme.css 172 B
build/block-library/blocks/shortcode/editor-rtl.css 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 744 B
build/block-library/blocks/site-logo/editor.css 744 B
build/block-library/blocks/site-logo/style-rtl.css 181 B
build/block-library/blocks/site-logo/style.css 181 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/social-link/editor-rtl.css 177 B
build/block-library/blocks/social-link/editor.css 177 B
build/block-library/blocks/social-links/editor-rtl.css 670 B
build/block-library/blocks/social-links/editor.css 669 B
build/block-library/blocks/social-links/style-rtl.css 1.32 kB
build/block-library/blocks/social-links/style.css 1.32 kB
build/block-library/blocks/spacer/editor-rtl.css 307 B
build/block-library/blocks/spacer/editor.css 307 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 471 B
build/block-library/blocks/table/editor.css 472 B
build/block-library/blocks/table/style-rtl.css 481 B
build/block-library/blocks/table/style.css 481 B
build/block-library/blocks/table/theme-rtl.css 188 B
build/block-library/blocks/table/theme.css 188 B
build/block-library/blocks/tag-cloud/style-rtl.css 146 B
build/block-library/blocks/tag-cloud/style.css 146 B
build/block-library/blocks/template-part/editor-rtl.css 560 B
build/block-library/blocks/template-part/editor.css 559 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 569 B
build/block-library/blocks/video/editor.css 570 B
build/block-library/blocks/video/style-rtl.css 173 B
build/block-library/blocks/video/style.css 173 B
build/block-library/blocks/video/theme-rtl.css 124 B
build/block-library/blocks/video/theme.css 124 B
build/block-library/common-rtl.css 910 B
build/block-library/common.css 908 B
build/block-library/editor-rtl.css 10 kB
build/block-library/editor.css 10 kB
build/block-library/reset-rtl.css 474 B
build/block-library/reset.css 474 B
build/block-library/style-rtl.css 10.9 kB
build/block-library/style.css 10.9 kB
build/block-library/theme-rtl.css 675 B
build/block-library/theme.css 679 B
build/block-serialization-default-parser/index.min.js 1.09 kB
build/block-serialization-spec-parser/index.min.js 2.79 kB
build/blocks/index.min.js 46.3 kB
build/components/index.min.js 215 kB
build/components/style-rtl.css 15.5 kB
build/components/style.css 15.5 kB
build/compose/index.min.js 11.2 kB
build/core-data/index.min.js 13.2 kB
build/customize-widgets/index.min.js 11.4 kB
build/customize-widgets/style-rtl.css 1.5 kB
build/customize-widgets/style.css 1.49 kB
build/data-controls/index.min.js 631 B
build/data/index.min.js 7.49 kB
build/date/index.min.js 31.9 kB
build/deprecated/index.min.js 485 B
build/dom-ready/index.min.js 304 B
build/dom/index.min.js 4.5 kB
build/edit-navigation/index.min.js 16 kB
build/edit-navigation/style-rtl.css 3.76 kB
build/edit-navigation/style.css 3.76 kB
build/edit-post/classic-rtl.css 492 B
build/edit-post/classic.css 494 B
build/edit-post/index.min.js 29.5 kB
build/edit-post/style-rtl.css 7.16 kB
build/edit-post/style.css 7.15 kB
build/edit-site/index.min.js 35.8 kB
build/edit-site/style-rtl.css 6.61 kB
build/edit-site/style.css 6.6 kB
build/edit-widgets/index.min.js 16.5 kB
build/edit-widgets/style-rtl.css 4.18 kB
build/edit-widgets/style.css 4.18 kB
build/editor/style-rtl.css 3.75 kB
build/editor/style.css 3.74 kB
build/element/index.min.js 3.29 kB
build/escape-html/index.min.js 517 B
build/format-library/index.min.js 6.58 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.63 kB
build/html-entities/index.min.js 424 B
build/i18n/index.min.js 3.71 kB
build/is-shallow-equal/index.min.js 501 B
build/keyboard-shortcuts/index.min.js 1.8 kB
build/keycodes/index.min.js 1.39 kB
build/list-reusable-blocks/index.min.js 1.72 kB
build/list-reusable-blocks/style-rtl.css 838 B
build/list-reusable-blocks/style.css 838 B
build/media-utils/index.min.js 2.92 kB
build/notices/index.min.js 925 B
build/nux/index.min.js 2.08 kB
build/nux/style-rtl.css 747 B
build/nux/style.css 743 B
build/plugins/index.min.js 1.84 kB
build/primitives/index.min.js 924 B
build/priority-queue/index.min.js 582 B
build/react-i18n/index.min.js 671 B
build/react-refresh-entry/index.min.js 8.44 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/redux-routine/index.min.js 2.65 kB
build/reusable-blocks/index.min.js 2.22 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 11 kB
build/server-side-render/index.min.js 1.57 kB
build/shortcode/index.min.js 1.49 kB
build/token-list/index.min.js 639 B
build/url/index.min.js 1.9 kB
build/viewport/index.min.js 1.05 kB
build/warning/index.min.js 248 B
build/widgets/index.min.js 7.15 kB
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.04 kB

compressed-size-action

@noisysocks
Copy link
Member

Hi @stacimc, thanks so much for digging into this. Are you still working on it?

@stacimc
Copy link
Contributor Author

stacimc commented Oct 29, 2021

@noisysocks I'm just picking this back up.

At the moment it matches designs proposed in the issue, except that it does not link to an external page to edit the Site Icon settings. I set this aside because I was expecting #29126 to add those settings somewhere in the Site editor or admin, where I could link to them.

At the moment I think the best I can do is link to the Customizer, since unfortunately I don't think it's possible to link to the Site Identity section specifically.

@stacimc stacimc added the [Block] Site Logo Affects the Site Logo Block label Oct 29, 2021
@stacimc stacimc marked this pull request as ready for review October 29, 2021 22:35
@stacimc stacimc requested a review from ajitbohra as a code owner October 29, 2021 22:35
@stacimc stacimc removed the [Status] In Progress Tracking issues with work in progress label Oct 29, 2021
@stacimc stacimc changed the title [WIP] Site Logo: Add option to set site icon from Site Logo block Site Logo: Add option to set site icon from Site Logo block Oct 29, 2021
const syncSiteIconHelpText = createInterpolateElement(
__(
"Site Icons are what you see in browser tabs, bookmark bars, and within the WordPress mobile apps. If you don't have one, you can set your logo to also be your icon. If you do have a custom site icon, you can upload that from the Site Icon settings in the <a>Customizer</a>!"
),
Copy link
Member

Choose a reason for hiding this comment

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

I have personal preference for less copy, especially seeing as the Site Logo block description covers a lot already, so this suggestion is 100% ignorable 😄

Toggle to set your logo to also be your icon. You can upload a custom site icon in the <a>Customizer</a>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm I was concerned with making sure that the distinction between icon and logo was clear here. You make a good point about the Site Logo block description -- I hadn't noticed it addresses the site icon in that text as well:

Display a graphic to represent this site. Update the block, and the changes apply everywhere it’s used. This is different than the site icon, which is the smaller image visible in your dashboard, browser tabs, etc used to help others recognize this site.

Knowing that it does feel a bit redundant, but I still think it's important to make sure the use of the toggle is clear from its own helptext, and I imagine not everyone will read through the block description 🤔

@ramonjd
Copy link
Member

ramonjd commented Oct 31, 2021

Thanks @stacimc ! So far, setting/resetting the logo and icon in the post mostly works for me in non-block and block themes (I tested Twenty Twenty One and TT1 Blocks).

The changes are synched to the Customizer and on the frontend.

I noticed an issue with new posts whereby I couldn't publish a new post if I deselect the site logo/icon options on the pre-publish panel.

Nov-01-2021.10-11-55.mp4

I'm not sure why it's occurring yet, but the error is this.props.setEntitiesSavedStatesCallback is not a function
It looks like the prop setEntitiesSavedStatesCallback is not passed to the post publish button from the post publish panel.

Note, I can replicate this on trunk as well so it's not related to this PR I think. I just thought I'd flag it here first while it's fresh 😄

Created an issue here: #36096

@stacimc
Copy link
Contributor Author

stacimc commented Nov 2, 2021

I noticed an issue with new posts whereby I couldn't publish a new post if I deselect the site logo/icon options on the pre-publish panel.
<...>
Note, I can replicate this on trunk as well so it's not related to this PR I think. I just thought I'd flag it here first while it's fresh 😄

@ramonjd This is a weird one, nice spot! I played around with this a bit and I can replicate it on trunk as well, and for that matter with other non-post entities like template parts, so I'm also confident this PR isn't related. Thanks for opening the issue, I'll add a comment there as well 👍

@ramonjd
Copy link
Member

ramonjd commented Nov 2, 2021

Swinging back to test this one further, I'm seeing a few things I didn't notice before.

The following occurs when I've already set the logo as my site icon in the block editor and saved the post.

Things appear as expected, but if I try to toggle the site icon off, without making any other changes, the editor state isn't dirty (and therefore not updatable) when toggling Use as site icon

Toggling other options triggers a dirty state.

Also, toggling the checkbox in the editor no longer appears to have any effect. I don't see the pre-publish save bar to confirm Site changes (not sure if I should), and the site icon remains on the site and in the Customizer.

I'm not if its related, but it looks like the dirtyRecords selector isn't being fired for some reason.

Can you reproduce this as well?

@stacimc
Copy link
Contributor Author

stacimc commented Nov 2, 2021

Swinging back to test this one further, I'm seeing a few things I didn't notice before.

The following occurs when I've already set the logo as my site icon in the block editor and saved the post.

Things appear as expected, but if I try to toggle the site icon off, without making any other changes, the editor state isn't dirty (and therefore not updatable) when toggling Use as site icon

If I'm understanding you correctly, what you're doing is:

  1. Sync the logo and icon using the toggle
  2. Save/publish the post
  3. Toggle the option off, but make no other changes
  4. Here you expect to be able to Update the post, but it appears that nothing has happened and you do not have the option to Update

I think this is working as I intended, but I can see why it may be confusing and am very open to suggestions for different behavior.

When you turn off the Use as site logo option, you indicate that any updates to the Site Logo shouldn't be synced to the icon, but it can't restore the icon to some previous value it may have had before syncing. The exception is if you have unsaved changes to the icon. So for example if I insert a Site Logo block, upload an image, and enable the toggle, as long as I turn the toggle off before saving, no changes will be made to the icon. But if changes have already been persisted, we'd need to keep track of previous values in order to restore them, which opens up a can of worms.

At the moment that option doesn't get persisted in an attribute or anything; when you load the block it is turned on if the logo/icon match.

@stacimc
Copy link
Contributor Author

stacimc commented Nov 2, 2021

Another thing we could do is clear the site icon when the Use logo as site icon option is toggled off. I actually had it doing this in an an initial pass but worried that might be unexpected behavior. What do you think @ramonjd?

@ramonjd
Copy link
Member

ramonjd commented Nov 3, 2021

When you turn off the Use as site logo option, you indicate that any updates to the Site Logo shouldn't be synced to the icon, but it can't restore the icon to some previous value it may have had before syncing.

Thanks for clarifying this point. This makes total sense when you say it like that 😄 So it's more of a "Use once, then keep in sync with site icon" sort of thing? :)

Armed with that revelation, I retested and can confirm that the switch toggles synching future changes to the site logo image. 👍

I'm not sure how to square it, but if I think of it as "Keep changes to the logo block in sync with the site icon", I still kinda expect the editor to allow me to save the state when I'm toggling off "Use as site icon". Even if it doesn't change/clear/affect the current value of the site icon.

I'm not saying this is the right expectation to have of course! :)

But if changes have already been persisted, we'd need to keep track of previous values in order to restore them, which opens up a can of worms

Ah good point. I clearly didn't think that far ahead. I think assumed the behaviour that you propose in this comment, but I agree that it might cause an unintentional clearing of the site icon.

It's a difficult one.

Maybe the toggle control label changes once a site logo has been selected, or changes. So if the current site logo is not the site icon, it'd say "Use as site icon", then after toggling to "on" it would say "Keep in sync". No idea if that's possible or even a good idea, or if it would make things clearer 🤷

@stacimc
Copy link
Contributor Author

stacimc commented Nov 3, 2021

I updated this to persist the state of the toggle in a block attribute. Here's how it affects @ramonjd's scenario:

  1. Insert a Site Logo block, upload a picture, and turn the toggle on. Save all entities.
  2. Toggle the Use as site icon switch off, but make no other changes.

New behavior:

  • Toggling the switch dirties the post, so you are able to Update.
  • If you do update and return to the post at a later time, the toggle will still be in the off position. If you change the Site Logo, it will not affect the Icon unless you deliberately toggle the option back on.
  • Toggling the option off in this scenario still won't affect or clear the actual Site Icon, because it has already been saved. It will clear any unsaved changes to the Icon.

The problem is this makes staying synced with changes made to the Icon in the Customizer more confusing. Scenario:

  • Update the Site Logo in the block editor and sync to the icon via the toggle. Save all entities.
  • Open the customizer and update the Site Icon.
  • Open the block editor again.

Changing the icon elsewhere feels like a deliberate action to un-sync the logo and icon, so I would expect the toggle to be off when I revisit the block editor, even though I last saved it in the on position. This is especially important because otherwise the page will be dirty as soon as you load it. I added some logic to detect this and reset the toggle to false if the icon has been changed.

I think I'm happy with that balance. @mtias can you weigh in here -- does this align with your idea?

@ramonjd
Copy link
Member

ramonjd commented Nov 3, 2021

Thanks for working on this @stacimc !

After retesting here's what I'm seeing now:

Nov-04-2021.09-24-57.mp4

✅ I inserted a new site logo and synched to my site icon. After saving, the site logo and icon match.
✅ In the post I toggled "Use as site icon" to off. The post allowed me to update and save my settings.
✅ I then replaced the site logo and saved. The site logo updated but the icon didn't change.

I also went through the testing scenarios again and things are working as described. 🎉

@ramonjd ramonjd added [Type] Enhancement A suggestion for improvement. [Type] Feature New feature to highlight in changelogs. labels Nov 4, 2021
@ramonjd ramonjd mentioned this pull request Nov 5, 2021
12 tasks
Previously we were getting the logoBlockCount on block creation, but
this value could become out of date if more Site Logo blocks were added
afterward.

Now we count the number of blocks at the time of block removal, so that it
is guaranteed up-to-date, and we ensure that changes are only discarded
if there are no other Site Logo blocks on the page.
@stacimc stacimc force-pushed the add/option-to-sync-site-icon-from-site-logo-block branch from 2410dd0 to e3d32bf Compare December 20, 2021 22:05
@stacimc
Copy link
Contributor Author

stacimc commented Dec 20, 2021

Thanks for all the testing! Rebased to hopefully pick up e2e test fixes, and I've updated the logic to discard unsaved changes when removing the final Site Logo block.

With this PR it's also possible to set a non-square site icon, if the site logo image isn't square

I would say that we should probably apply an overflow rule to the W button though, to stop that strange on-hover behaviour.

I think this is consistent with the behavior currently on trunk. I can test setting a non-square Site Logo by going through the Customizer and clicking Skip cropping when I select a non-square image from my media library.

The behavior on hover is also in trunk, even when I select a Site Icon through the Customizer settings page and crop it appropriately. It looks like the animation is fairly new from #33132, and the outline comes from #29888. I do think that the outline looks unusual when a non-square icon is hovered:
Screen Shot 2021-12-20 at 2 40 08 PM

@stacimc stacimc requested a review from mkaz as a code owner December 20, 2021 22:49
@stacimc
Copy link
Contributor Author

stacimc commented Dec 20, 2021

Not sure what's up with all the failing tests after rebase, taking another look now 👀

Edit: Updated the docs! The other tests should be good when we're able to re-run 🤞

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Great work @stacimc! I've re-tested this, and I can't reproduce any of the unsaved changes issues that I ran into previously. I believe they've all been fixed up via moving getGlobalBlockCount to its own useSelect (and calling it at the last minute when needed), and shifting the check for logoBlockCount to be against 0 instead of 1. This is testing really well 🙂

It'd be good to fix up the styling on the W button, but given the progress already in this PR, I'd be in favour of merging in this PR as-is and us then looking into the button styling in a follow-up, considering it's already a bug on trunk.

LGTM! 🎉

@jameskoster
Copy link
Contributor

I do think that the outline looks unusual when a non-square icon is hovered

Yeah that was my main concern. I see that the "outline" is a pseudo element, so a simple overflow style will not fix this. We can probably address it in a follow up.

@stacimc stacimc merged commit e329250 into trunk Dec 21, 2021
@stacimc stacimc deleted the add/option-to-sync-site-icon-from-site-logo-block branch December 21, 2021 19:29
@github-actions github-actions bot added this to the Gutenberg 12.3 milestone Dec 21, 2021
@stacimc
Copy link
Contributor Author

stacimc commented Dec 21, 2021

I do think that the outline looks unusual when a non-square icon is hovered

Yeah that was my main concern. I see that the "outline" is a pseudo element, so a simple overflow style will not fix this. We can probably address it in a follow up.

@jameskoster Agreed! Issue created: #37564

@stacimc stacimc 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 Dec 21, 2021
@noisysocks noisysocks 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 Jan 4, 2022
noisysocks pushed a commit that referenced this pull request Jan 4, 2022
* Add toggle for syncing site icon to site logo

* Sync icon when first uploading logo, if no icon exists

* Do not clear the site icon when toggling off syncing, instead restore to original icon

* Link to Customizer from Site Logo syncing help text

* Sync site icon on uploading new image or resetting image

* Simplify logic

* Automatically sync icon if none exists and adding logo for the first time

* Add translated string for site icon property

* Link directly to Site Icon settings in Customizer

* Adjust copy

* Correct opening customizer in new tab

* Persist toggle state in an attribute

* Turn the toggle off if the logo and icon fall out of sync

If a user syncs the logo and icon via the toggle and saves changes, but later
changes the site icon in the Customizer, we want the toggle to be reset to the
`off` position when they next edit the block.

* Discard unsaved changes to logo and icon when removing last Logo block

* Prevent uncontrolled input when sync attribute undefined

* Tighten up copy

* Force syncing on initial selection

* Get up-to-date logoBlockCount when removing block

Previously we were getting the logoBlockCount on block creation, but
this value could become out of date if more Site Logo blocks were added
afterward.

Now we count the number of blocks at the time of block removal, so that it
is guaranteed up-to-date, and we ensure that changes are only discarded
if there are no other Site Logo blocks on the page.

* Add shouldSyncIcon attr to core blocks documentation
const { editEntityRecord } = useDispatch( coreStore );
const setLogo = ( newValue ) =>

useEffect( () => {
Copy link
Contributor

@youknowriad youknowriad Jan 10, 2022

Choose a reason for hiding this comment

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

Hi there!

The logic here doesn't make sense, it's creating bugs like #37760

Basically the logic here assumes than when we "unmount" the site logo block, we're removing it when it's not always the case, unmounting can happen in several ways:

  • Switching to code editor in post editor
  • Changing the current view (template/template part) in the site editor.

While I'm not sure about the rest of this PR, this effect should be removed.

Copy link
Member

@ramonjd ramonjd Jan 11, 2022

Choose a reason for hiding this comment

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

@stacimc I removed the hook and went through the testing instructions above under "Test discarding unsaved changes:"

I couldn't find any weird side effects, that is, the editor recognised and kept my updates when viewing previews and when switching between code/visual modes.

In other words, things still work for me.

Is there something I'm not testing right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logic here doesn't assume that unmounting the block means we're removing it. It runs every time the block is unmounted, but it then checks the global block count to determine whether the Site Logo blocks have actually been removed; otherwise it should do nothing.

I think the problem is in the Site Editor, that getGlobalBlockCount isn't behaving the way I believed it would. Although all my own test scenarios still work for me, I'm able to reproduce the #37760 bug by:

  • Add a Site Logo block and make a change in the Header part
  • Add a different block to a different template part which does not contain a Site Logo block
  • Hit Undo and see that the 'Redo' button is disabled

When the Site logo block unmounts, getGlobalBlockCount returns 0 this time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll put up a PR to remove the logic for now and then start looking into a better way of doing this. I do think it would be nice to discard those unsaved changes when the block is removed, if I can get it working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#37895 fixes the regression, and #37900 has been opened to investigate adding the functionality back in safely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @stacimc getGlobalBlockCount is not reliable because the core/block-editor store is more like an internal store to an instance of the block editor and don't say anything about the presence of a block in a template (or all templates). It's just specific to the current state of the screen.

@mehne
Copy link

mehne commented Apr 21, 2022

As all Threads are closed and redirected here for "Can't add favicon without customizer", and we only get the sulotion by either visit the customize.php manually or use the Site-Icon block

#29126
and
https://core.trac.wordpress.org/ticket/54370

I need to post this here:

  1. Manually visit customize.php is not a good solution for non-tech people
  2. Only beeing able to set the favicon via the Logo-Block is not viable cause in many cases the Logo is not a square or in some cases there isn't even a logo added through the FSE

So there should definetly be another solution to add a favicon for non-tech users.

I think the FSE / Gutenberg was added to reduce the custom solutions made by Themes.
But I'm afraid there are gonna be multiple custom solutions even through (unneeded) plugins to add the favicon in a more central and user-friendly way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Site Logo Affects the Site Logo Block [Type] Enhancement A suggestion for improvement. [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.