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

feat(splashscreen): Add support for spinner on Android and iOS #1653

Merged
merged 13 commits into from
Jun 19, 2019

Conversation

trancee
Copy link
Contributor

@trancee trancee commented Jun 11, 2019

This pull request will add support for displaying a spinner on top of the splash screen, both for Android and iOS.

There are some new configuration keys:

  • showSpinner bool: if true will enable the spinner, defaults to false.
  • spinnerStyle string: controls the style of the spinner.
    Android: horizontal, small, large, inverse, smallInverse, largeInverse.
    iOS < 13: whiteLarge, large, gray.
    iOS >= 13: large, medium.
  • spinnerColor string: sets the color of the spinner, values are either #RGB or #ARGB.

Refers also to issue #928.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

Added a comment.
And a few more things:

  • Use 2 spaces instead of 4
  • As most spinner names are different per platform, better use different config values for them, like androidSpinnerStyle and iosSpinnerStyle, otherwise will not work unless the user changes it every time.
  • On iOS you have launchSpinnerStyle and launchSpinnerColor that are not mentioned on the PR explanation and not present on Android. Why is that?
  • Don't add iOS 13 code yet, it's still on beta and might change. Also makes the tests to fail because that code is not available on Xcode 10.
  • Document the new preferences in Splash docs

@@ -1,149 +1,270 @@
import Foundation
import AudioToolbox

// https://stackoverflow.com/questions/24263007/how-to-use-hex-color-values
extension UIColor {
Copy link
Member

Choose a reason for hiding this comment

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

There is an UIColor extension already (UIColor.swift) with hex implementation, so move it all there. It's not good to have it inside another class, nor have hex duplicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback, @jcesarmobile.

Please have a look at the changes, I hope I made it all right this time. Let me know if you have any other suggestions.

@trancee
Copy link
Contributor Author

trancee commented Jun 12, 2019

@jcesarmobile I am using swift-format as suggested in this blog post:
https://nshipster.com/swift-format/#swift-format

This is the only one that uses 2 spaces instead of 4. Even if do copy/paste of code in Xcode itself, it will change to 4 spaces. I am not sure what kind of code formatter you are using, but I am open for any suggestion to do it the proper way.

@jcesarmobile
Copy link
Member

I don't use any formatter.
You can configure Xcode to use 2 spaces instead of 4 globally, or set it per file before editing it (probably this is better for you as you probably use 4 in other places)
See screenshot showing both the global preference and the per file on the right side
Captura de pantalla 2019-06-13 a las 15 10 24

@trancee
Copy link
Contributor Author

trancee commented Jun 14, 2019

@jcesarmobile Thank you for the suggestion, it works that way in Xcode with 2 spaces indentation.

@jcesarmobile jcesarmobile changed the title Added support for spinner on Android and iOS. feat(splashscreen): Add support for spinner on Android and iOS Jun 19, 2019
@jcesarmobile
Copy link
Member

I've made a few code changes on the iOS code fixing some mistakes and code formatting.

Also simplified the iOS styles to just large and small, making large the default to match android default, because as they can be colored I think it doesn't make sense to have a grey option that you can change to another color.

Android code looks good, going to merge, thanks!

@jcesarmobile jcesarmobile merged commit 650e0cf into ionic-team:master Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants