apache / cordova-ios

Apache Cordova iOS
https://cordova.apache.org/
Apache License 2.0
2.15k stars 987 forks source link

feat!: Use single 1024x1024 icon #1309

Closed jcesarmobile closed 1 year ago

jcesarmobile commented 1 year ago

Xcode 14 allows to use a single 1024x1024 icon instead of having all the icon sizes.

It's breaking because it requires Xcode 14, but since Xcode 14.1 is going to be required for app store submissions starting the 25 of April, I think we are good.

Removed the icon sizes link because it was broken

Closes #1019 closes #1233

dpogue commented 1 year ago

We should make sure this works properly if someone just puts <icon src="res/ios/icon.png" /> in their config.xml (with no sizes specified).

There's a part of me that thinks it would be good to have some support for customizing the notification and watch icons, but that would involve dynamically adding them to the .xcassets Contents.json only if they were specified, which feels like it would be error-prone if people are mixing manual changes and automatic ones 😞

codecov-commenter commented 1 year ago

Codecov Report

Merging #1309 (1b6a41b) into master (3d6c71a) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1309   +/-   ##
=======================================
  Coverage   78.48%   78.48%           
=======================================
  Files          15       15           
  Lines        1780     1780           
=======================================
  Hits         1397     1397           
  Misses        383      383           
Impacted Files Coverage Δ
lib/prepare.js 85.02% <ø> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

jcesarmobile commented 1 year ago

If using single icon, Xcode only allows a single icon, doesn’t allow to add a different icon notifications. It allows a different icon for watchOS, but at the moment it was using the same as for iOS

dpogue commented 1 year ago

Ahh okay

dpogue commented 1 year ago

Note: Closes #1019 & closes #1233

breautek commented 1 year ago

It's also breaking in the sense it requires iOS 12+

Screenshot 2023-05-03 at 9 48 57 AM

So I think we also need to bump CordovaLib min target to iOS 12, which doesn't have to be part of this PR, but it will need to be set for this feature.

breautek commented 1 year ago

FWIW, we've gone back in our apps and just supplied the icons that were missing (so that we aren't relying on a feature that may not even make it into release).

This appears to cover all current icons, at least for the iPhone models.

erisu commented 1 year ago

I believe it was decided to staying on iOS 11, so I think this PR can be closed. Is a nice idea though. Maybe in a future release.