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

Fix maskable icon setting behaviour based on site icon #770

Merged
merged 5 commits into from
Apr 23, 2022

Conversation

thelovekesh
Copy link
Collaborator

This is a follow-up to #304 and was originally discussed in #304 (comment).

This PR aims to:

  • Add logic to hide the Maskable Icon checkbox if site icon is not set.
  • Add logic to remove checked from Maskable Icon checkbox if the user has removed the site icon.

* Hide the maskable icon setting if site icon is not set
* Set maskable icon value to false if site icon is not set.
@thelovekesh thelovekesh requested a review from westonruter April 21, 2022 15:42
@maitreyie-chavan maitreyie-chavan linked an issue Apr 22, 2022 that may be closed by this pull request
@westonruter westonruter added this to the 0.7 milestone Apr 23, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2022

Codecov Report

Merging #770 (d19d86e) into develop (441f16f) will increase coverage by 0.05%.
The diff coverage is 76.92%.

@@              Coverage Diff              @@
##             develop     #770      +/-   ##
=============================================
+ Coverage      18.96%   19.01%   +0.05%     
  Complexity       346      346              
=============================================
  Files             57       57              
  Lines           2320     2324       +4     
=============================================
+ Hits             440      442       +2     
- Misses          1880     1882       +2     
Flag Coverage Δ
php 19.01% <76.92%> (+0.05%) ⬆️
unit 19.01% <76.92%> (+0.05%) ⬆️

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 66.37% <71.42%> (-0.15%) ⬇️
wp-includes/class-wp-customize-manager.php 88.88% <83.33%> (-3.12%) ⬇️

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 441f16f...d19d86e. Read the comment docs.

Comment on lines +34 to +40
// Change the site_icon_maskable if site_icon is not set.
siteIconSetting.bind((newSiteIconValue) => {
if (!newSiteIconValue) {
siteIconMaskableSetting(false);
}
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

This does capture @pooja-muchandikar's comment in #304 (comment):

If Site icon was set and set as maskable and then removed the site icon, Maskable Icon setting remains checked.

However, I didn't think this was necessarily a bug. It's just whether one thinks of the maskable property being linked to the icon, or if maskable is something that you would persist across icon changes.

Copy link
Collaborator Author

@thelovekesh thelovekesh Apr 23, 2022

Choose a reason for hiding this comment

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

However, I didn't think this was necessarily a bug.

Agreed, rather it is just a check to ensure that by default we are assuming that an icon is maskable.

If we had to save maskable property in an icon meta then we will be able to guess it correctly, for an icon maskable property should be true or not, but saving maskable property in meta could be overkill IMO.

@westonruter
Copy link
Collaborator

In d19d86e I've addressed @pooja-muchandikar's point in #304 (comment):

  • Maskable checkbox is in place but when the checkbox is checked from customizer, an error is shown on manifest, ideally it should show any error. Also the same error is displayed twice any reason?
    image

image

Now when I set the maskable checkbox, I no longer get the warnings:

image

DevTools seems to group the icons with duplicate dimensions horizontally (although it strangely doesn't automatically apply the mask to them).

@westonruter westonruter merged commit dd833d2 into develop Apr 23, 2022
@westonruter westonruter deleted the fix/maskable-icon-setting branch April 23, 2022 01:06
@pooja-muchandikar
Copy link

QA Passed ✅

No warnings are displayed if maskable checkbox is checked.

Screenshot 2022-04-25 at 10 03 36 AM


Backend settings are also working fine as expected.

Screen.Recording.2022-04-25.at.9.56.40.AM.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Facilitate indicating the site icon as being maskable
4 participants