ETK Nav Sidebar: Use site icon for menu toggle when available

[creativecoder]

Changes proposed in this Pull Request

Use the site icon for toggling the editor sidebar menu (when in full screen mode), rather then the default "W" icon.

Related to #51396
Related to #51482
Resolves #51740

Closed	Open

Testing instructions

Testing this requires install the Editing Toolkit plugin, v10.3.0 or higher of the Gutenberg plugin, and wpcomsh (for dot org sites). I recommend a8c-wp-env (for installing all dependencies), or installing the plugin zip to an Atomic site (PCYsg-ly5-p2#etk-and-atomic-sites).

Note that the sidebar menu styles were changed in v10.3 of Gutenberg (the current release). If you wanted to be thorough, you could test with a previous version of the plugin as well (I did while I was developing and it looked okay).

1. Set a site icon, using either the Customizer or Settings (Calpyso version)
2. Open a page or post in the editor
3. You should see the site icon used to toggle the sidebar.
4. Ensure focus and hover styles, as well as keyboard navigation, work as expected

Note that this requires D59822-code to work for Simple sites.

[matticbot]

Test live: https://calypso.live/?branch=add/site-icon-to-nav-sidebar

[matticbot]

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

[ciampo]

I left a few inline comments, but overall this PR tests well as per instructions, including hover and focus styles

Tested on an Atomic site with Gutenberg 10.3.1 (my site's icon is an orange square):

etk-sidebar-icon.mp4

Also, it seems like the unit tests CI task is failing, which is not ideal — could you try rebasing to see it if fixes it?

[ciampo]

TypeScript is not happy with the third argument being undefined:

Argument of type 'undefined' is not assignable to parameter of type 'number'.

[creativecoder]

I resolved the type error, but it's a bit ugly! I'd imagine the types will be updated in Gutenberg if and when the __unstableBase becomes "stable".

[creativecoder]

Also, it seems like the unit tests CI task is failing, which is not ideal — could you try rebasing to see it if fixes it?

Done, I think that did it.

@ciampo Thanks so much for the detailed feedback. This is ready for another look.

[creativecoder]

Note that I've created a diff that gets this working with Simple sites, as well: D59822-code

[ciampo]

Tests well as per instructions, and all feedback has been addressed (I just added a tiny change that addressed a potential flexbox bug).

I'm going to take over this PR and merge it, since it fixes #51740.

Also, noting that D59822-code needs to be merged in order for the icon to display correctly in Simple Sites

[creativecoder]

Thanks for getting this over the line @ciampo ! ❤️