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

(iOS) Dark mode splashscreen storyboard images #808

Merged
merged 4 commits into from
Jun 17, 2020
Merged

Conversation

terreng
Copy link
Contributor

@terreng terreng commented Mar 10, 2020

This is my first pr so please don't burn me at the stake.

Platforms affected

iOS

Motivation and Context

I wanted to include a dark mode variant of my app's splash screen, but found that Cordova did not yet support this feature. It turns out that it only took a few lines of code, so I figured I'd share it here.

I found this related issue: apache/cordova-plugin-splashscreen#246. It turns out that no changes to the splashscreen plugin are necessary to make it happen.

Description

This enables dark mode storyboard images by allowing you to specify splash images based on appearance (dark or light). It checks for ~dark or ~light suffixes in splash images. If neither is found, they're used as the default. It then adds the variants to Contents.json, and the rest is handled by iOS. Example config.xml:

<splash src="res/screen/ios/Default@3x~iphone~comany.png" />
<splash src="res/screen/ios/Default@3x~iphone~comany~dark.png" />

This pr only adds support for iOS.

Testing

I've tested these changes on an iPhone and everything appears to work as intended. I've updated existing tests and added new ones as appropriate, and all tests pass.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

Documentation would need to change over at https://github.com/apache/cordova-plugin-splashscreen/blob/master/README.md.

Thank you.

Check for ~dark and ~light suffixes, and include in Contents.json.
@NiklasMerz
Copy link
Member

@terreng Thank you for your first PR 🥇 This is a great feature. Please check the Travis log, if you can turn the tests green, review is much easier.

Looks like some tests need to be changed and/or new tests are needed.

@breautek
Copy link
Contributor

Thank you for your PR. Just a couple notes, not exactly related to this PR, but when making PRs in general.

I haven't run any tests

You can run local tests on your machine by running npm test. This will help catch some problems (such as your eslint error 😜 )

I also noticed that you made changes in your fork's master branch for your PR. I would recommend always creating a new branch for your PRs to keep your master branch clean. This isn't a problem right now, but could be later. If you add more commits to your master (maybe for a different PR) those commits will end up in this PR as well and that would not be very desirable.

Regarding this PR specifically, I think everything looks good and I'm happy you considered making specifying the appearance optional, which also helps maintain backwards compatibility. As soon as the checks turn green I'll give my 👍

@terreng
Copy link
Contributor Author

terreng commented Mar 11, 2020

Alright, I've updated the failed tests and added three new ones.

@codecov-io
Copy link

Codecov Report

Merging #808 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #808      +/-   ##
==========================================
+ Coverage    74.2%   74.28%   +0.08%     
==========================================
  Files          13       13              
  Lines        1849     1855       +6     
==========================================
+ Hits         1372     1378       +6     
  Misses        477      477
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/prepare.js 82.28% <100%> (+0.23%) ⬆️

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 20248c1...c7c2b6e. Read the comment docs.

@breautek breautek added this to the 6.0.0 milestone Mar 11, 2020
@breautek breautek requested a review from NiklasMerz March 11, 2020 03:27
@NiklasMerz
Copy link
Member

I think documentation how to use this feature should be part of this PR, too.

@terreng
Copy link
Contributor Author

terreng commented Mar 14, 2020

Documentation would have to change for cordova-plugin-splashscreen which isn't a part of this repo.

@NiklasMerz
Copy link
Member

Good point. I got confused that the code is part of cordova-ios and not the plugin.

@dpogue dpogue modified the milestones: 6.0.0, 6.0.1 Jun 11, 2020
Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

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

Did some tests with this and it all looks to work as intended 👍

Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

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

Code Changes LGTM

@dpogue dpogue merged commit 55ee692 into apache:master Jun 17, 2020
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.

6 participants