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.

Sign up for GitHub

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

Automatically copy name to short_name in web app manifest if not greater than 12 characters #212

Merged
merged 5 commits into from
Aug 20, 2019
Merged

Automatically copy name to short_name in web app manifest if not greater than 12 characters #212

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
606fc6c
Automatically copy name to short_name in web app manifest if <= 12 chars
westonruter Aug 16, 2019
ab28700
Add site health check for short_name in web app manifest
westonruter Aug 16, 2019
5d78752
Update since tags
westonruter Aug 16, 2019
0d41abd
Set dist to trusty
westonruter Aug 19, 2019
7b40cb5
Use placeholders in translation strings
westonruter Aug 20, 2019
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
2 changes: 1 addition & 1 deletion pwa.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ function _pwa_add_disabled_navigation_preload_site_status_test( $tests ) {
add_filter( 'site_status_tests', '_pwa_add_disabled_navigation_preload_site_status_test' );

/**
* Print admin notice when a build has not been been performed.
* Flag navigation preload incorrectly being disabled.
*
* This is temporary measure to correct a mistake in the example for how navigation request caching strategies.
*
Expand Down
10 changes: 9 additions & 1 deletion tests/test-class-wp-web-app-manifest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public function test_get_theme_color() {
*/
public function test_get_manifest() {
$this->mock_site_icon();
$blogname = "PWA's Domain Is Here";
$blogname = 'PWA Test';
update_option( 'blogname', $blogname );
$actual_manifest = $this->instance->get_manifest();

Expand All @@ -139,6 +139,7 @@ public function test_get_manifest() {
'description' => get_bloginfo( 'description' ),
'display' => 'minimal-ui',
'name' => $blogname,
'short_name' => $blogname,
'lang' => get_bloginfo( 'language' ),
'dir' => is_rtl() ? 'rtl' : 'ltr',
'start_url' => home_url( '/' ),
Expand All @@ -147,6 +148,13 @@ public function test_get_manifest() {
);
$this->assertEquals( $expected_manifest, $actual_manifest );

// Check that long names do not automatically copy to short name.
$blogname = str_repeat( 'x', 13 );
update_option( 'blogname', $blogname );
$actual_manifest = $this->instance->get_manifest();
$this->assertEquals( $blogname, $actual_manifest['name'] );
$this->assertArrayNotHasKey( 'short_name', $actual_manifest );

// Test that the filter at the end of the method overrides the value.
add_filter( 'web_app_manifest', array( $this, 'mock_manifest' ) );
$actual_manifest = $this->instance->get_manifest();
Expand Down
105 changes: 105 additions & 0 deletions wp-includes/class-wp-web-app-manifest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,16 @@ class WP_Web_App_Manifest {
*/
const REST_ROUTE = '/web-app-manifest';

/**
* Maximum length for short_name.
*
* @since 0.4
* @link https://developers.google.com/web/tools/lighthouse/audits/manifest-contains-short_name
* @link https://developer.chrome.com/apps/manifest/name#short_name
* @var int
*/
const SHORT_NAME_MAX_LENGTH = 12;

/**
* The default manifest icon sizes.
*
Expand All @@ -51,6 +61,7 @@ class WP_Web_App_Manifest {
public function init() {
add_action( 'wp_head', array( $this, 'manifest_link_and_meta' ) );
add_action( 'rest_api_init', array( $this, 'register_manifest_rest_route' ) );
add_filter( 'site_status_tests', array( $this, 'add_short_name_site_status_test' ) );
}

/**
Expand Down Expand Up @@ -123,6 +134,19 @@ public function get_manifest() {
'display' => 'minimal-ui',
'dir' => is_rtl() ? 'rtl' : 'ltr',
);

/*
* If the name is 12 characters or less, use it as the short_name. Lighthouse complains when the short_name
* is absent, even when the name is 12 characters or less. Chrome's max recommended short_name length is 12
* characters.
*
* https://developers.google.com/web/tools/lighthouse/audits/manifest-contains-short_name
* https://developer.chrome.com/apps/manifest/name#short_name
*/
if ( strlen( $manifest['name'] ) <= self::SHORT_NAME_MAX_LENGTH ) {
$manifest['short_name'] = $manifest['name'];
}

$language = get_bloginfo( 'language' );
if ( $language ) {
$manifest['lang'] = $language;
Expand Down Expand Up @@ -155,6 +179,87 @@ public function get_manifest() {
return apply_filters( 'web_app_manifest', $manifest );
}

/**
* Register test for lacking short_name in web app manifest.
*
* @since 0.4
*
* @param array $tests Tests.
* @return array Tests.
*/
public function add_short_name_site_status_test( $tests ) {
$tests['direct']['web_app_manifest_short_name'] = array(
'label' => __( 'Short Name in Web App Manifest', 'pwa' ),
'test' => array( $this, 'test_short_name_present_in_manifest' ),
);
return $tests;
}

/**
* Test that web app manifest contains a short_name.
*
* @since 0.4
* @todo Add test for PNG site icon.
*
* @return array Test results.
*/
public function test_short_name_present_in_manifest() {
$manifest = $this->get_manifest();

/* translators: %d is the max length as a number */
$description = sprintf( __( 'The <code>short_name</code> is a short version of your website&#8217;s name which is displayed when there is not enough space for the full name, for example with the site icon on a phone&#8217;s homescreen. It should be a maximum of %d characters long.', 'pwa' ), self::SHORT_NAME_MAX_LENGTH );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd use a placeholder for short_name. Also, the first sentence is very long. I'd split it up to make translators' lives easier.


$actions = __( 'You currently may use <code>web_app_manifest</code> filter to set the short name, for example in your theme&#8217;s <code>functions.php</code>.', 'pwa' );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, I'd use placeholders for the filter and file name here.


if ( empty( $manifest['short_name'] ) ) {
$result = array(
'label' => __( 'Web App Manifest lacks a short_name entry', 'pwa' ),
'status' => 'recommended',
'badge' => array(
'label' => __( 'Progressive Web App', 'pwa' ),
'color' => 'orange',
),
'description' => wp_kses_post( sprintf( '<p>%s</p>', $description ) ),
'actions' => wp_kses_post( $actions ),
);
} elseif ( strlen( $manifest['short_name'] ) > self::SHORT_NAME_MAX_LENGTH ) {
$result = array(
'label' =>
sprintf(
/* translators: %1$s is the short name */
__( 'Web App Manifest has a short_name (%s) that is too long', 'pwa' ),
esc_html( $manifest['short_name'] )
),
'status' => 'recommended',
'badge' => array(
'label' => __( 'Progressive Web App', 'pwa' ),
'color' => 'orange',
),
'description' => wp_kses_post( sprintf( '<p>%s</p>', $description ) ),
'actions' => wp_kses_post( $actions ),
);
} else {
$result = array(
'label' =>
sprintf(
/* translators: %1$s is the short name */
__( 'Web App Manifest has a short_name (%s)', 'pwa' ),
esc_html( $manifest['short_name'] )
),
'status' => 'good',
'badge' => array(
'label' => __( 'Progressive Web App', 'pwa' ),
'color' => 'green',
),
'description' => wp_kses_post( sprintf( '<p>%s</p>', $description ) ),
);
}

$result['test'] = 'web_app_manifest_short_name';

return $result;
}

/**
* Registers the rest route to get the manifest.
*/
Expand Down