Handle the site_icon_maskable setting within a full-site-editing context
#919
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Thank you @westonruter for all your feedback. |
…roduced as a parameters on the wp-scripts calls
|
What do you think about the current state @westonruter ? |
|
|
||
| // Stop, if needed file was not built successfully. | ||
| $script_asset_path = "$path/site-icon-maskable.asset.php"; | ||
| if ( ! file_exists( $script_asset_path ) ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This is a redundant check with _pwa_print_build_needed_notice, right? So is it needed?
| // Stop, if needed file was not built successfully. | |
| $script_asset_path = "$path/site-icon-maskable.asset.php"; | |
| if ( ! file_exists( $script_asset_path ) ) { | |
| return; | |
| } |
There was a problem hiding this comment.
You are right, that is a redundant check, but leaving it out will result in fatal error with the following require statement on the next line. Or am I wrong?
Additionally, because this is called inside the block-editor, where the normal admin_notices will not be visible, this should prevent errors while trying to enque a non-existent file. To help the user mentioning, that the plugin was not built properly, we could introduce one of the newer createWarningNotice for the block-editor. But this should be new issue, I think.
There was a problem hiding this comment.
You are right, that is a redundant check, but leaving it out will result in fatal error with the following
requirestatement on the next line. Or am I wrong?
If you see the change you made in pwa.php:
Lines 100 to 106 in 81d2860
Note that in the bootstrap, it does a return to prevent the rest of the PWA functionality from being loaded. So if the file doesn't exist, then this conditional won't ever be reached.
wp-includes/js/src/site-icon-maskable/maskable-icon-controls.js
Outdated
Show resolved
Hide resolved
| 'site_icon_maskable' | ||
| ); | ||
|
|
||
| // mainly borrowed from ... |
There was a problem hiding this comment.
| // mainly borrowed from ... | |
| // mainly borrowed from the `SiteIcon` component |
Unfortunately there is no nice docs-page to reference to, propably because this component is part of the Edit Site components, which are described in its own README as
[...] but please keep in mind that it might never get fully documented.
| <PanelBody> | ||
| <Flex align="start"> | ||
| <FlexBlock> | ||
| <ToggleControl |
There was a problem hiding this comment.
When I toggle this on the first tine I get a React warning:
Warning: A component is changing an uncontrolled input to be controlled. This is likely caused by the value changing from undefined to a defined value, which should not happen. Decide between using a controlled or uncontrolled input element for the lifetime of the component. More info: https://reactjs.org/link/controlled-components
Perhaps this is because it is missing a default false value?
| if (siteIconUrl) { | ||
| siteIcon = ( | ||
| <img | ||
| alt={__('Site Icon')} | ||
| className="components-site-icon" | ||
| src={siteIconUrl} | ||
| width={64} | ||
| height={64} | ||
| style={siteIconStyle} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
I tried selecting a non-square image as the Site Logo and this is resulting in an unexpected result for the maskable icon:
Three things aren't quite right here:
- The toggle for a maskable icon is showing even if I've not selected that I want to use the Site Logo as the Site Icon. I should only see the toggle if I'm using the Site Logo as the Site Icon.
- A Site Logo need not be square, but a Site Icon must be. Nevertheless, WP allows selection of a rectangular icon but then it crops it to be square. The same logic used for cropping to be square should also be replicated here in the preview. (Aside: I'm surprised that core doesn't have such a Site Icon preview when the "Use as site icon" toggle is checked, as there should be a way to see whether the square crop will even work. This would be an improvement for core.)
- If I change the selected image for the Site Logo while the block controls are open, the preview for the maskable icon does not update to use the newly selected image. See below:
There was a problem hiding this comment.
OMG. But you are totally right on all of your points.
I'll try to find some solutions.
| const [siteIconMaskable, setSiteIconMaskable] = useEntityProp( | ||
| 'root', | ||
| 'site', | ||
| 'site_icon_maskable' |
There was a problem hiding this comment.
This is unexpectedly showing up as the label in the pre-save dialog when it should have a label:
There was a problem hiding this comment.
This is unfortunately a known bug in gutenberg, I'm going to ref. the issue here, when I'll find it again ;)
|
Relatedly, I just filed WordPress/gutenberg#48608 |
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #919 +/- ##
=============================================
+ Coverage 19.62% 19.64% +0.02%
- Complexity 347 349 +2
=============================================
Files 57 57
Lines 2324 2347 +23
=============================================
+ Hits 456 461 +5
- Misses 1868 1886 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Introduce additional UI for the
core/site-logo-block to toggle the existing setting and give a preview of the Image choosen, with the setting applied.This should help deprecating the customizer at all, without loosing functionaliy and flexibility for the user.
This fixes #915