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

Handle the site_icon_maskable setting within a full-site-editing context #919

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
/phpcs.xml
/.phpcs.xml
*.zip
/wp-includes/js/dist/*
/wp-includes/js/workbox*
/wiki
.vscode
Expand Down
1 change: 1 addition & 0 deletions .phpcs.xml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
<exclude-pattern>./node_modules/</exclude-pattern>
<exclude-pattern>./vendor/</exclude-pattern>
<exclude-pattern>./build/</exclude-pattern>
<exclude-pattern>./wp-includes/js/dist/</exclude-pattern>

<!-- How to scan -->
<arg value="sp"/> <!-- Show sniff and progress -->
Expand Down
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@
"workbox-cli": "6.5.4"
},
"scripts": {
"build": "grunt build; grunt create-build-zip",
"build": "npm run block:build; grunt build; grunt create-build-zip",
"block:build": "wp-scripts build site-icon-maskable=./wp-includes/js/src/site-icon-maskable/index.js --output-path=wp-includes/js/dist",
"block:start": "wp-scripts start site-icon-maskable=./wp-includes/js/src/site-icon-maskable/index.js --output-path=wp-includes/js/dist",
"check-engines": "wp-scripts check-engines",
"check-licenses": "wp-scripts check-licenses --production",
"deploy": "grunt deploy",
Expand Down
6 changes: 5 additions & 1 deletion pwa.php
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ function _pwa_incorrect_plugin_slug_admin_notice() {
* Print admin notice when a build has not been been performed.
*
* @since 0.2
* @since 0.8.0-alpha Check for site-icon-maskable.asset.php, which should be auto-generated by wp-scripts
*/
function _pwa_print_build_needed_notice() {
?>
Expand All @@ -96,7 +97,10 @@ function _pwa_print_build_needed_notice() {
</div>
<?php
}
if ( ! file_exists( PWA_PLUGIN_DIR . '/wp-includes/js/workbox-v' . PWA_WORKBOX_VERSION ) || ! file_exists( PWA_PLUGIN_DIR . '/wp-includes/js/workbox-v' . PWA_WORKBOX_VERSION . '/workbox-sw.js' ) ) {
if ( ! file_exists( PWA_PLUGIN_DIR . '/wp-includes/js/workbox-v' . PWA_WORKBOX_VERSION ) ||
! file_exists( PWA_PLUGIN_DIR . '/wp-includes/js/workbox-v' . PWA_WORKBOX_VERSION . '/workbox-sw.js' ) ||
! file_exists( PWA_PLUGIN_DIR . '/wp-includes/js/dist/site-icon-maskable.asset.php' )
) {
add_action( 'admin_notices', '_pwa_print_build_needed_notice' );
return;
}
Expand Down
24 changes: 18 additions & 6 deletions tests/test-class-wp-web-app-manifest.php
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ public function test_init() {
$this->assertEquals( 10, has_action( 'wp_head', array( $this->instance, 'manifest_link_and_meta' ) ) );
$this->assertEquals( 10, has_action( 'rest_api_init', array( $this->instance, 'register_manifest_rest_route' ) ) );

$this->assertEquals( 10, has_action( 'rest_api_init', array( $this->instance, 'register_short_name_setting' ) ) );
$this->assertEquals( 10, has_action( 'admin_init', array( $this->instance, 'register_short_name_setting' ) ) );
$this->assertEquals( 10, has_action( 'rest_api_init', array( $this->instance, 'register_settings' ) ) );
$this->assertEquals( 10, has_action( 'admin_init', array( $this->instance, 'register_settings' ) ) );
$this->assertEquals( 10, has_action( 'admin_init', array( $this->instance, 'add_short_name_settings_field' ) ) );
}

Expand Down Expand Up @@ -532,23 +532,35 @@ public function test_get_url() {
}

/**
* Test register_short_name_setting.
* Test register_settings.
*
* @covers ::register_short_name_setting()
* @covers ::register_settings()
*/
public function test_register_short_name_setting() {
public function test_register_settings() {
global $wp_registered_settings;
unset( $wp_registered_settings['short_name'] );

$this->assertArrayNotHasKey( 'short_name', $wp_registered_settings );
$this->instance->register_short_name_setting();
$this->instance->register_settings();
$this->assertArrayHasKey( 'short_name', $wp_registered_settings );
$setting = $wp_registered_settings['short_name'];

$this->assertEquals( 'string', $setting['type'] );
$this->assertEquals( 'general', $setting['group'] );
$this->assertEquals( array( $this->instance, 'sanitize_short_name' ), $setting['sanitize_callback'] );
$this->assertEquals( true, $setting['show_in_rest'] );

unset( $wp_registered_settings['site_icon_maskable'] );

$this->assertArrayNotHasKey( 'site_icon_maskable', $wp_registered_settings );
$this->instance->register_settings();
$this->assertArrayHasKey( 'site_icon_maskable', $wp_registered_settings );
$setting = $wp_registered_settings['site_icon_maskable'];

$this->assertEquals( 'boolean', $setting['type'] );
$this->assertEquals( 'general', $setting['group'] );
$this->assertEquals( 'rest_sanitize_boolean', $setting['sanitize_callback'] );
$this->assertEquals( true, $setting['show_in_rest'] );
}

/**
Expand Down
72 changes: 68 additions & 4 deletions wp-includes/class-wp-web-app-manifest.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,11 @@ public function init() {
add_action( 'rest_api_init', array( $this, 'register_manifest_rest_route' ) );
add_filter( 'site_status_tests', array( $this, 'add_pwa_site_health_tests' ) );

add_action( 'rest_api_init', array( $this, 'register_short_name_setting' ) );
add_action( 'admin_init', array( $this, 'register_short_name_setting' ) );
add_action( 'rest_api_init', array( $this, 'register_settings' ) );
add_action( 'admin_init', array( $this, 'register_settings' ) );
add_action( 'admin_init', array( $this, 'add_short_name_settings_field' ) );
add_action( 'enqueue_block_editor_assets', array( $this, 'enqueue_site_icon_maskable_block_editor_assets' ) );

}

/**
Expand Down Expand Up @@ -533,9 +535,15 @@ public static function get_url() {
}

/**
* Register setting for short_name.
* Register pwa settings to the settings-API and make them visible to the REST API.
*
* @since 0.7.0
* @since 0.8.0-alpha Added registration of 'site_icon_maskable' setting.
*
* @return void
*/
public function register_short_name_setting() {
public function register_settings() {
// Register setting for short_name.
register_setting(
'general',
self::SHORT_NAME_OPTION,
Expand All @@ -546,6 +554,29 @@ public function register_short_name_setting() {
'show_in_rest' => true,
)
);

/*
* Register setting for maskable site-icon.
*
* Even that this option is not exposed
* to the settings UI within normal admin options pages,
* the registration is needed to make the option
* available to the REST API, which is used by
* the site-editor.
*
* In the site-editor, this option can be set with the help of
* UI that is added to the 'site-logo' core block.
*/
register_setting(
'general',
'site_icon_maskable',
array(
'type' => 'boolean',
'description' => __( 'Wether the current site icon is maskable or not, as this is needed by some devices.', 'pwa' ),
'sanitize_callback' => 'rest_sanitize_boolean',
'show_in_rest' => true,
)
);
}

/**
Expand Down Expand Up @@ -677,4 +708,37 @@ function updateShortNameField() {
</script>
<?php
}

/**
* Load 'wp/site-logo'-block filter
*
* Handle the 'site_icon_maskable' setting in a full-site-editing context,
* by enqueing a filter to the 'wp:site-logo' block within the site-editor.
*
* @since 0.8.0-alpha
* @see https://developer.wordpress.org/reference/hooks/enqueue_block_editor_assets/
*
* @return void
*/
public function enqueue_site_icon_maskable_block_editor_assets() {
$dir = '/wp-includes/js/dist';
$path = PWA_PLUGIN_DIR . $dir;

// Stop, if needed file was not built successfully.
$script_asset_path = "$path/site-icon-maskable.asset.php";
if ( ! file_exists( $script_asset_path ) ) {
return;
}
Comment on lines +726 to +731
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a redundant check with _pwa_print_build_needed_notice, right? So is it needed?

Suggested change
// Stop, if needed file was not built successfully.
$script_asset_path = "$path/site-icon-maskable.asset.php";
if ( ! file_exists( $script_asset_path ) ) {
return;
}

Copy link
Author

@carstingaxion carstingaxion Mar 2, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

If you see the change you made in pwa.php:

pwa-wp/pwa.php

Lines 100 to 106 in 81d2860

if ( ! file_exists( PWA_PLUGIN_DIR . '/wp-includes/js/workbox-v' . PWA_WORKBOX_VERSION ) ||
! file_exists( PWA_PLUGIN_DIR . '/wp-includes/js/workbox-v' . PWA_WORKBOX_VERSION . '/workbox-sw.js' ) ||
! file_exists( PWA_PLUGIN_DIR . '/wp-includes/js/dist/site-icon-maskable.asset.php' )
) {
add_action( 'admin_notices', '_pwa_print_build_needed_notice' );
return;
}

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.


$script_asset = require $script_asset_path;
$index_js = "$dir/site-icon-maskable.js";
wp_enqueue_script(
'pwa-site-icon-maskable-block-editor',
plugins_url( $index_js, PWA_PLUGIN_FILE ),
$script_asset['dependencies'],
$script_asset['version'],
true
);
wp_set_script_translations( 'pwa-site-icon-maskable-block-editor', 'pwa' );
}
}
45 changes: 45 additions & 0 deletions wp-includes/js/src/site-icon-maskable/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/**
* The 'icon maskable' control allows to set the 'site_icon_maskable' option.
*
* It also provides a small preview of the image used as Site-Logo,
* using a cropping-preview to illustrate the safe space an logo needs
* to be an adaptive image.
*/
import MaskableControls from './maskable-icon-controls';

/**
* The compose package is a collection
* of handy Hooks and Higher Order Components (HOCs)
* you can use to wrap your WordPress components
* and provide some basic features like: state, instance id, pure…
*
* @see https://developer.wordpress.org/block-editor/reference-guides/packages/packages-compose/
*/
import { createHigherOrderComponent } from '@wordpress/compose';

const withMaskableControls = createHigherOrderComponent((BlockEdit) => {
return (props) => {
if (props.name !== 'core/site-logo') {
return <BlockEdit {...props} />;
}

return (
<>
<BlockEdit {...props} />
<MaskableControls />
</>
);
};
}, 'withMaskableControls');

/**
* To modify the behavior of existing blocks,
* WordPress exposes several APIs.
*
* @see https://developer.wordpress.org/block-editor/reference-guides/filters/block-filters/
*/
wp.hooks.addFilter(
'editor.BlockEdit',
'pwa/with-maskable-icon-controls',
withMaskableControls
);
115 changes: 115 additions & 0 deletions wp-includes/js/src/site-icon-maskable/maskable-icon-controls.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
/**
* Retrieves the translation of text.
*
* @see https://developer.wordpress.org/block-editor/packages/packages-i18n/
*/
import { __ } from '@wordpress/i18n';

/**
* This module allows you to create and use standalone block editors. ;)
*
* @see https://developer.wordpress.org/block-editor/reference-guides/packages/packages-block-editor/
*/
import { InspectorControls } from '@wordpress/block-editor';

/**
* This package includes a library of generic WordPress components
* to be used for creating common UI elements shared between screens
* and features of the WordPress dashboard.
*
* @see https://developer.wordpress.org/block-editor/reference-guides/packages/packages-components/
*/
import {
Flex,
FlexBlock,
FlexItem,
PanelBody,
ToggleControl,
} from '@wordpress/components';

/**
* Core Data is a data module intended to
* simplify access to and manipulation
* of core WordPress entities.
*
* @see https://developer.wordpress.org/block-editor/reference-guides/packages/packages-core-data/
*/
import { store as coreStore, useEntityProp } from '@wordpress/core-data';

/**
* WordPress’ data module serves as a hub
* to manage application state
* for both plugins and WordPress itself.
*
* @see https://developer.wordpress.org/block-editor/reference-guides/packages/packages-data/
*/
import { useSelect } from '@wordpress/data';

export default function MaskableControls() {
const [siteIconMaskable, setSiteIconMaskable] = useEntityProp(
'root',
'site',
'site_icon_maskable'
Copy link
Collaborator

@westonruter westonruter Mar 1, 2023

Choose a reason for hiding this comment

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

This is unexpectedly showing up as the label in the pre-save dialog when it should have a label:

image

Copy link
Author

Choose a reason for hiding this comment

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

This is unfortunately a known bug in gutenberg, I'm going to ref. the issue here, when I'll find it again ;)

);

// mainly borrowed from ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

From what?

Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
// mainly borrowed from ...
// mainly borrowed from the `SiteIcon` component

SiteIcon

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.

const { isRequestingSiteIcon, siteIconUrl } = useSelect((select) => {
const { getEntityRecord, isResolving } = select(coreStore);
const siteData =
getEntityRecord('root', '__unstableBase', undefined) || {};

return {
isRequestingSiteIcon: isResolving('getEntityRecord', [
'root',
'__unstableBase',
undefined,
]),
siteIconUrl: siteData.site_icon_url,
};
}, []);

if (isRequestingSiteIcon) {
return null;
}

const siteIconStyle = {
clipPath: siteIconMaskable ? 'inset(10% round 50%)' : '',
width: '64px',
};

let siteIcon = <div style={siteIconStyle} />;

if (siteIconUrl) {
siteIcon = (
<img
alt={__('Site Icon')}
className="components-site-icon"
src={siteIconUrl}
width={64}
height={64}
style={siteIconStyle}
/>
);
}
Comment on lines +82 to +93
Copy link
Collaborator

Choose a reason for hiding this comment

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

I tried selecting a non-square image as the Site Logo and this is resulting in an unexpected result for the maskable icon:

image

Three things aren't quite right here:

  1. 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.
  2. 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.)
  3. 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:

image

Copy link
Author

Choose a reason for hiding this comment

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

OMG. But you are totally right on all of your points.

I'll try to find some solutions.


return (
<InspectorControls>
<PanelBody>
<Flex align="start">
<FlexBlock>
<ToggleControl
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

label={__('Maskable icon', 'pwa')}
help={__(
'Maskable icons let your Progressive Web App use adaptive icons. If you supply a maskable icon, your icon can fill up the entire shape as an app- or homescreen-icon and will look great on all devices.',
'pwa'
)}
onChange={setSiteIconMaskable}
checked={siteIconMaskable}
/>
</FlexBlock>
<FlexItem>{siteIcon}</FlexItem>
</Flex>
</PanelBody>
</InspectorControls>
);
}