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: Add SplashScreen plugin #149

Merged
merged 5 commits into from
Dec 19, 2020
Merged

feat: Add SplashScreen plugin #149

merged 5 commits into from
Dec 19, 2020

Conversation

jcesarmobile
Copy link
Member

@jcesarmobile jcesarmobile commented Dec 18, 2020

closes #133
closes #150

splash-screen/src/web.ts Show resolved Hide resolved
splash-screen/src/web.ts Show resolved Hide resolved
splash-screen/CONTRIBUTING.md Outdated Show resolved Hide resolved
@carlpoole
Copy link
Member

Couple of ideas

SplashScreenConfig and SplashScreenSettings could use Builders so the resulting object is immutable - it looks like those are only accessed inside SplashScreen and not set there.

the show method in SplashScreen might also benefit from a method signature without a Settings parameter so it just uses a default SplashScreenSettings. (similar to the old one that just took an activity like this:
public static void show(final Activity a) { show(a, DEFAULT_LAUNCH_SHOW_DURATION, DEFAULT_FADE_IN_DURATION, DEFAULT_FADE_OUT_DURATION, DEFAULT_AUTO_HIDE, null, null); })

@jcesarmobile
Copy link
Member Author

As the idea of the implementation class is to make it easier to reuse the plugin by native code, it's easier to have a single object rather than a bunch of params.
Also, iOS had a TODO about the show having too many parameters, which wasn't good for the linter and the TODO comment suggested to use a struct, so I did. So on Android I used classes since there are no structs.

My code is based on Ian's approach on camera PR, which was approved by you and Dan, so if we want to use builders his PR should also be changed.

I removed the show with no params because it was not being used and iOS didn't have an equivalent, but I can put it back if you want. But then iOS should also have something like that for consistency.

@carlpoole
Copy link
Member

I removed the show with no params because it was not being used and iOS didn't have an equivalent, but I can put it back if you want. But then iOS should also have something like that for consistency.

I'm thinking from a reusability standpoint for this method:

show(final AppCompatActivity activity, final SplashScreenSettings settings, final SplashListener splashListener)

You would call it:

show(activity, new SplashScreenSettings(), listener)

or you could also do

show(activity, listener)

if you just want defaults. If the first one is acceptable then that's fine.

@imhoffd imhoffd merged commit c5f44be into main Dec 19, 2020
Capacitor Engineering ⚡️ automation moved this from Needs review 🤔 to Done 🎉 Dec 19, 2020
@imhoffd imhoffd deleted the splash branch December 19, 2020 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

SplashScreen functions have extraneous callback parameter Memory leak?
3 participants