-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat: add monochrome app icon support #1550
Conversation
Here is the screenshot showing the themed icon for a starter project Icon credits: @Birowsky |
caea0bc
to
95d4cdb
Compare
Looks like a great start. I wonder if we can update the updateIcons function so that we can optionally specify a monochrome icon via the A full example may look like: <icon
density="xxxhdpi"
background="adaptive_bg.png"
foreground="adaptive_fg.png"
monochrome="monochrome.png"
src="xxxhdpi.png"
/> Traditionally we always supported both rasterized images as well as vectors, but if monochrome doesn't support a rasterized image then we will just need to make sure we note that when we document the feature. The android docs does mention that adaptive icons are required to be declared if using the monochrome, so if the
Hopefully the existing code is readable enough that the pattern can be followed, e.g. copying the icon resource to the appropriate drawable folder by density, then writing the changes into the adaptive icon xml. |
Thank you for the feedback @breautek !
I was aiming to solve the first use case as documented in the description of the PR. In this scenario, I did read through I'll add a note to my TODO to make sure we update the docs as required. |
@breautek need your inputs please. I'm updating the
but it isn't present in the icons returned from
I tried to do grep search to see the definition of Would you advise where can I find the |
I was able to figure this out. This function calls
FYI. |
@breautek I think I've finished the implementation for support for monochrome icons. Kindly review the code. Apart from changes in this project, I had to do a small change in So far, I've covered two use cases - (i) Cordova Starter project created with A sample Cordova project was created to test use case 2, which can be found here. If everything looks good, we'll have to update the docs. Please let me know your comments. |
@breautek can you please advise the next steps? One aspect I know of is updating the docs. Any pointers regarding this would be helpful. |
@liyamahendra The file that you would need to edit is here: https://github.com/apache/cordova-docs/blob/master/www/docs/en/dev/config_ref/images.md This page contains the current documentation for Adaptive Icons. |
In addition to what Erisu said, please have a look at the unit tests, there are a few lint checks that are failing.
I think the overall test will fail until we have the cordova-common changes merged and released, but we should be able to correct the lint errors in the meantime. These are all trivial errors that eslint should be able to fix automatically using |
I think the changes look good, at this point I think we just need to wait for a cordova-common@5 release for apache/cordova-common#197 I'll do a more proper review when all the dependent pieces are all in place. |
package.json
Outdated
@@ -26,7 +26,7 @@ | |||
"license": "Apache-2.0", | |||
"dependencies": { | |||
"android-versions": "^1.7.0", | |||
"cordova-common": "^4.0.2", | |||
"cordova-common": "^4.0.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liyamahendra Can you revert this accidental change?
Also rebase with master after. I have released [email protected], bumped the package, and merged into master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erisu please review it again, when you can. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some reason the rebase didn't look correct.
If it is OK, and if I can push, can I try and rebase your branch to fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! Please feel free to push and rebase my branch as required. Thank you for asking!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liyamahendra sorry for the delayed response.
I had reverted and rebased the branch correctly. It no longer shows changes that doesnt belong in this PR.
You will most likely need to force pull the changes since it was a rebase.
I will review the changes again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can also review the commit changes one time to confirm nothing was lost, that will be great.
So far everything looks great on my end.
There is one test failure,
✗ Test#010 : Should detech adaptive icon with vector foreground and throws error for missing backwards compatability settings. (0.013 sec)
- Expected function to throw CordovaError: For the following icons with the density of: mdpi, adaptive foreground with a defined color or vector can not be used as a standard fallback icon for older Android devices. To support older Android environments, please provide a value for the src attribute., but it threw Error: ENOENT: no such file or directory, open '/home/runner/work/cordova-android/cordova-android/platforms/android/app/src/main/res/mipmap-mdpi-v26/ic_launcher.xml'.
I believe it is related to this change:
It seems the .xml
file extension was changed to .png
while the test is related to vector foreground
. I think this is an incorrect change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erisu you're right - changing the extension was causing the test to fail. Corrected that change.
Please have a look and let me know if there is anything else.
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final call for reviews before merging. |
Thank you @liyamahendra for helping us provide the monochrome theme icon feature. |
Platforms affected
Motivation and Context
Android 13 added support for Themed Icons. Since themed icons aren't yet supported on Cordova Android, adding support for Themed icons.
closes #1450
Description
I'm considering the below two use cases:
config.xml
for android platformTesting
Checklist
(platform)
if this change only applies to one platform (e.g.(android)
)