Add maskable icon checkbox in customizer settings#698
Conversation
… as maskable * Lighthouse gives an error if icon is not maskable. * Add a checkbox to allow user to save icon as maskable icon. * If user have not opt-in for maskable icon then manifest will add icon purpose as `any maskable`.
Codecov Report
@@ Coverage Diff @@
## develop #698 +/- ##
=============================================
+ Coverage 19.05% 19.75% +0.69%
- Complexity 324 326 +2
=============================================
Files 55 56 +1
Lines 2062 2086 +24
=============================================
+ Hits 393 412 +19
- Misses 1669 1674 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| 'pwa_maskable_icon', | ||
| array( | ||
| 'capability' => 'manage_options', | ||
| 'type' => 'option', |
There was a problem hiding this comment.
Since it is a specific icon that is maskable, and the icon is an image uploaded to the media library, I wonder if it would be preferable to store this maskable flag as postmeta instead. That may be overkill.
There was a problem hiding this comment.
@westonruter site_icon is getting store in *_options table and maskable property is associated with site_icon itself. IMO, it makes sense to store maskable in options as there will be only one site_icon in customizer.
There was a problem hiding this comment.
But if someone picks a different image for the site icon, then it may not be maskable. So that's why I was thinking it could be useful to associate the “maskability“ with the image attachment post specifically. But this would add quite a bit of complexity and it isn't necessary as the user would clearly see that the icon is maskable or not upon selecting a new image.
There was a problem hiding this comment.
@westonruter Before your code changes, when user was picking new image for site icon, I was resetting the maskable toggle value.
Here, we can consider that every newly uploaded image may not be maskable and if user want he can explicitly mark the maskable input.
Only drawback of this approach is next time user selects previously uploaded icon, check box will not be marked/unmarked based on last input for that icon.
IMO, considering the tradeoff of simplicity vs experience, for now we can proceed with options and may change it in future if such requests are raised.
|
@westonruter If all seems good, can we proceed with merging this one? |
westonruter
left a comment
westonruter
left a comment
There was a problem hiding this comment.
Yes, sorry for the delay.
4 participants
Closes #304