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

Add short name field in General Settings when Site Title is longer than 12 characters #691

Merged
merged 15 commits into from
Feb 11, 2022

Conversation

westonruter
Copy link
Collaborator

@westonruter westonruter commented Feb 8, 2022

Fixes #210.

This adds a new Short Name field to the General Settings screen:

image

This field is required if the Site Title is longer than 12 characters:

image

If if the Site Title is less than or equal to 12 characters long, then the placeholder value is set to match the Site Title (since it is used by default in the manifest in this case), and the field is not required:

image

If the Site Title is greater than 12 characters in length, a user must supply a Short Name to save the form (and the field has a maxlength of 12 supplied, which is enforced by the sanitize_callback):

image

If a site is supplying the Short Name via a plugin, like as follows (which was the only way to do so before now):

add_filter( 'web_app_manifest', static function ( $manifest ) {
	$manifest['short_name'] = 'WP Dev';
	return $manifest;
} );

Then the Short Name field is disabled in the same way the Site URL and Home URL fields if they are defined with constants:

image

The Site Health test for the Short Name now includes a link to the General Settings screen to manage the field:

image

Other changes:

  • Use mb_strlen() for short_name check.
  • Use trim() on a short name before checking the length or adding to the manifest.
  • Enable client-side form validation on the General Settings screen. (It was disabled for a bug in an 8-year old version of Firefox, per TRAC-22183.)

@westonruter westonruter added this to the 0.7 milestone Feb 8, 2022
@westonruter westonruter marked this pull request as ready for review February 8, 2022 21:33
@westonruter
Copy link
Collaborator Author

westonruter commented Feb 8, 2022

Still need to add tests, but otherwise this is ready for review.

  • Add tests.

@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2022

Codecov Report

Merging #691 (0e850c7) into develop (928905e) will increase coverage by 2.30%.
The diff coverage is 83.95%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #691      +/-   ##
=============================================
+ Coverage      16.75%   19.05%   +2.30%     
- Complexity       305      324      +19     
=============================================
  Files             55       55              
  Lines           2011     2062      +51     
=============================================
+ Hits             337      393      +56     
+ Misses          1674     1669       -5     
Flag Coverage Δ
php 19.05% <83.95%> (+2.30%) ⬆️
unit 19.05% <83.95%> (+2.30%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
wp-includes/class-wp-web-app-manifest.php 73.88% <83.95%> (+14.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 928905e...0e850c7. Read the comment docs.

@westonruter westonruter added the web-app-manifest Web App Manifests label Feb 8, 2022
@westonruter
Copy link
Collaborator Author

westonruter commented Feb 8, 2022

  • Add link from Site Health test to Short Name field on General Settings screen.

Copy link
Collaborator

@thelovekesh thelovekesh left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@westonruter westonruter merged commit 80780c4 into develop Feb 11, 2022
@westonruter westonruter deleted the add/short-name-field branch February 11, 2022 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Short Name as admin settings field alongside Site Title
3 participants