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

CB-13143: Integrate and replace SplashScreens with Launch Storyboard #790

Merged
merged 5 commits into from
May 21, 2020

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Feb 18, 2020

Platforms affected

iOS

Motivation and Context

  1. Apple is deprecating Launch Images and will require Launch Storyboards for all apps as of April 2020 (Closes Use storyboards & default split screens support - required from July 2020 #770).

  2. As per the decisions resulting from the plugin audit, CB-13143 intends to integrate the SplashScreen plugin API into each native platform and deprecate the plugin.

Description

  • Remove the default Launch Images
  • Remove support for Launch Images from the prepare step
  • Integrate the required functionality for showing/hiding a screenshot of the Launch Storyboard to CDVViewController
  • Add a built-in plugin with the same API as the SplashScreen plugin

Testing

  • Manually created a test app and verified the launch storyboard behaviour.
  • Manually confirmed the SplashScreen API works as intended from the JS side.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • 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

@dpogue dpogue requested a review from erisu February 18, 2020 04:53
@erisu
Copy link
Member

erisu commented Feb 18, 2020

@dpogue I pushed an update to remove the unit tests that I forgot to remove in the commit that you cherry picked. refactor (prepare): delete legacy splash screen launch images. The Jasmine unit tests should be OK now.

Locally I saw another error but from the Objective-C tests.

@dpogue
Copy link
Member Author

dpogue commented Feb 19, 2020

@erisu FYI I rebased and squashed your Jasmine test fix commit into the one where the prepare code was removed.

This should hopefully pass all the Obj-C tests too

@codecov-io
Copy link

codecov-io commented Feb 19, 2020

Codecov Report

Merging #790 into master will increase coverage by 0.43%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #790      +/-   ##
==========================================
+ Coverage    74.2%   74.63%   +0.43%     
==========================================
  Files          13       13              
  Lines        1849     1770      -79     
==========================================
- Hits         1372     1321      -51     
+ Misses        477      449      -28
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/prepare.js 85.71% <100%> (+3.65%) ⬆️

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 2922437...a1d5c03. Read the comment docs.

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.

LGTM

@erisu erisu self-requested a review February 19, 2020 05:49
@erisu erisu force-pushed the launch-storyboard branch from f18db9e to f27f4cb Compare February 20, 2020 05:17
@dpogue dpogue force-pushed the launch-storyboard branch from f27f4cb to a1d5c03 Compare March 3, 2020 05:28
@erisu
Copy link
Member

erisu commented Mar 3, 2020

During testing, this PR will also no longer provide or display the default Cordova splashscreen as designed. Same behaviour as Android platform to not show a default splashscreen.

Copy link
Member

@NiklasMerz NiklasMerz left a comment

Choose a reason for hiding this comment

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

Code LGTM 👍 . Simple test was successfull (if you read the instructions about the iOS simulator in the splashscreen repo :-)). The loading circle in the splashscreen is gone, too.

Does documentation need to move to this repo as well?

@erisu
Copy link
Member

erisu commented Mar 15, 2020

Documentation would need to go into cordova-doc.

Only the plugin documentation were written within its own repos and then the cordova-doc repo would grab it. Platform docs were manually managed directly in cordova-doc.

@dpogue dpogue force-pushed the launch-storyboard branch from a1d5c03 to 23cfa1a Compare May 21, 2020 06:01
@codecov-commenter
Copy link

codecov-commenter commented May 21, 2020

Codecov Report

Merging #790 into master will increase coverage by 0.44%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #790      +/-   ##
==========================================
+ Coverage   74.40%   74.85%   +0.44%     
==========================================
  Files          13       13              
  Lines        1821     1742      -79     
==========================================
- Hits         1355     1304      -51     
+ Misses        466      438      -28     
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/prepare.js 85.71% <100.00%> (+3.65%) ⬆️

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 43e3bbd...23cfa1a. Read the comment docs.

@erisu erisu merged commit 2d8e0a9 into apache:master May 21, 2020
@kanongil
Copy link

kanongil commented Jun 12, 2020

This displays an empty screen for me (transparent, unless backgroundcolor is set). The storyboard is loaded and ok, but the view returned from snapshotViewAfterScreenUpdates: is blank.

/**
Method to be called from the plugin JavaScript to show or hide the launch screen.
*/
- (void)showLaunchScreen:(BOOL)visible

Choose a reason for hiding this comment

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

This method delays showing the splash screen by SplashScreenDelay ms when visible = YES.

@erisu
Copy link
Member

erisu commented Jun 12, 2020

@kanongil Please test against nightly or master.

The Issue you are mentioning sound relatively similar to what has already been reported and fixed in master.

If you have any issues please open a new ticket instead of messaging inside a PR which has already been merged and released.

@apache apache locked as resolved and limited conversation to collaborators Jun 12, 2020
@dpogue dpogue deleted the launch-storyboard branch September 22, 2021 02:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use storyboards & default split screens support - required from July 2020
8 participants