breautek commented on code in PR #1550:
URL: https://github.com/apache/cordova-android/pull/1550#discussion_r1086667148
##########
lib/prepare.js:
##########
@@ -728,17 +731,21 @@ function updateIconResourceForAdaptive (preparedIcons,
resourceMap, platformReso
// project's config.xml location, so we use it as base path.
let background;
let foreground;
+ let monochrome;
let targetPathBackground;
let targetPathForeground;
+ let targetPathMonochrome;
for (const density in android_icons) {
let backgroundVal = '@mipmap/ic_launcher_background';
let foregroundVal = '@mipmap/ic_launcher_foreground';
+ let monochromeVal = '@mipmap/ic_launcher_monochrome';
background = android_icons[density].background;
foreground = android_icons[density].foreground;
+ monochrome = android_icons[density].monochrome;
- if (!background || !foreground) {
+ if (!background || !foreground || !monochrome) {
Review Comment:
I think it's still valid to have an adaptive icon without monochrome and
this check I think will break that as if `background` and `foreground` are
defined, but `monochrome` isn't it will end up skipping the entire adaptive
icon block.
This check was here because you cannot have an adaptive icon without both
`background` and `foreground`. If I recall correctly from the android docs for
monochrome icons, it is an addon to adaptive icons, and using a monochrome also
requires the adaptive `background` / `foreground` drawables.
So in otherwords, you may have an adaptive icon without monochrome, but if
you have monochrome, then you must have the other adaptive icon properties as
well.
I think the `|| !monochrome` can be removed from this condition, and I think
we will have to have a `if (monochrome)` condition later.
##########
lib/prepare.js:
##########
@@ -814,6 +842,16 @@ function updateIconResourceForAdaptive (preparedIcons,
resourceMap, platformReso
defaultTargetPathForeground =
getAdaptiveImageResourcePath(platformResourcesDir, 'mipmap', 'mdpi',
'ic_launcher_foreground.png', path.basename(default_icon.foreground));
resourceMap[defaultTargetPathForeground] = default_icon.foreground;
}
+
+ if (path.extname(path.basename(monochrome)) === '.xml') {
+ // Vector Use Case
+ defaultTargetPathMonochrome =
getAdaptiveImageResourcePath(platformResourcesDir, 'mipmap', 'mdpi',
'ic_launcher_monochrome.xml', path.basename(default_icon.monochrome));
+ resourceMap[defaultTargetPathMonochrome] = default_icon.monochrome;
+ } else if (path.extname(path.basename(monochrome)) === '.png') {
+ // Images Use Case
+ defaultTargetPathMonochrome =
getAdaptiveImageResourcePath(platformResourcesDir, 'mipmap', 'mdpi',
'ic_launcher_monochrome.png', path.basename(default_icon.monochrome));
+ resourceMap[defaultTargetPathMonochrome] = default_icon.monochrome;
+ }
Review Comment:
I think this also needs to be contained in a `if (monochrome)` block for the
same reasons above.
##########
package.json:
##########
@@ -26,7 +26,7 @@
"license": "Apache-2.0",
"dependencies": {
"android-versions": "^1.7.0",
- "cordova-common": "^4.0.2",
+ "cordova-common": "https://github.com/liyamahendra/cordova-common";,
Review Comment:
If it requires a change in cordova-common, that's fine but just writing a
note here so that we don't merge the PR in while this is set to your fork.
It just means that we will have to get a PR into cordova-common and a build
a release before we can merge this PR in.
##########
lib/prepare.js:
##########
@@ -769,12 +776,32 @@ function updateIconResourceForAdaptive (preparedIcons,
resourceMap, platformReso
resourceMap[targetPathForeground] =
android_icons[density].foreground;
}
+ if (path.extname(path.basename(monochrome)) === '.xml') {
+ // Vector Use Case
+ targetPathMonochrome =
getAdaptiveImageResourcePath(platformResourcesDir, 'mipmap', density,
'ic_launcher_monochrome.xml', path.basename(android_icons[density].monochrome));
+ resourceMap[targetPathMonochrome] =
android_icons[density].monochrome;
+ } else if (path.extname(path.basename(monochrome)) === '.png') {
+ // Images Use Case
+ targetPathMonochrome =
getAdaptiveImageResourcePath(platformResourcesDir, 'mipmap', density,
'ic_launcher_monochrome.png', path.basename(android_icons[density].monochrome));
+ resourceMap[targetPathMonochrome] =
android_icons[density].monochrome;
+ }
Review Comment:
I think this if/elseif block needs to be contained in a `if (monochrome)`
block, given my review points above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
