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

fixes#881 - added splashscreen #913

Merged
merged 1 commit into from
May 20, 2020
Merged

Conversation

devansh-299
Copy link
Collaborator

Issue Fix

Fixes #881

Screen Recording

GIF-200518_194127

  • Apply the AndroidStyle.xml style template to your code in Android Studio.

  • Run the unit tests with ./gradlew check to make sure you didn't break anything

  • If you have multiple commits please combine them into one commit by squashing them.


<activity
android:name=".auth.ui.LoginActivity"
android:theme="@style/AppTheme"
Copy link
Contributor

Choose a reason for hiding this comment

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

@devansh-299 Just popped out of my mind, did you have to add the entire LoginActivity again. I remember we having it. Perhaps the change only is enough. I might be wrong as well. Please correct me if I am :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Bhavnaharitsa I didn't add LoginActivity again, I just removed Launcher filter from this activity and used SplashScreenActivity as the new Launcher activity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yess, I agree. But the commit message shows as such :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yess, I agree. But the commit message shows as such :)

yeah, the Github's change tracker is a little bit misleading

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, same happened with me in the previous pr GitHub is surely misleading

@devansh-299
Copy link
Collaborator Author

@Rachittt please have a look

@shiv07tiwari
Copy link
Member

@devansh-299 Maybe we can check the permissions and internet connection in the background while the user is looking into the splash screen. So isn't it better to use the Timer instead of using the theme and Cold Start time which can't be changed as per our requirements?
Your thoughts @luckyman20 @Bhavnaharitsa

@s-ayush2903
Copy link
Contributor

@devansh-299 Maybe we can check the permissions and internet connection in the background while the user is looking into the splash screen.

(This is just regarding the internet connection method)
Days back, I planned of implementing some networkChecker method in the module but surfing in the repo, I encountered this review on the same idea. I was planning of implementing that method as some singleton(like those method which we have in Utils class) and using it in the module wherever required.
So, is it needed/required now? I'll be eager to file a PR regarding the same.
Also, one more thing as discussed/pointed out previously in old discussions (like

that Utils class again is quite small and easier to be ktfied. Is it allowed?

@Bhavnaharitsa
Copy link
Contributor

@shiv07tiwari Yes, setting up a timer is by far beneficial as it would help get us the required permissions the app, and like you said sir, the internet connectivities as well.
@s-ayush2903 You mean, we'd be using or interfacing the singleton class wherever the internet connectivities are required? If that's the case, then we can use that in the Splash screen to check if the user has a valid connection to proceed to the SignIn process because right after the SplashScreen comes the SignIn page which I guess requires a good internet connection.

@devansh-299
Copy link
Collaborator Author

@devansh-299 Maybe we can check the permissions and internet connection in the background while the user is looking into the splash screen. So isn't it better to use the Timer instead of using the theme and Cold Start time which can't be changed as per our requirements?
Your thoughts @luckyman20 @Bhavnaharitsa

@shiv07tiwari The basic application can run without having the read/write and camera
permissions, so should we need necessarily need to ask for these permissions before hand?

@devansh-299
Copy link
Collaborator Author

@shiv07tiwari @Bhavnaharitsa @s-ayush2903 I don't know whether this - introduction slides functionality is really something we need, but I once implemented this for Mifos Mobile PR where we can ask users for necessary permissions and give a basic introduction to the application.

@s-ayush2903
Copy link
Contributor

@s-ayush2903 You mean, we'd be using or interfacing the singleton class wherever the internet connectivities are required? If that's the case, then we can use that in the Splash screen to check if the user has a valid connection to proceed to the SignIn process because right after the SplashScreen comes the SignIn page which I guess requires a good internet connection.

No, not interfacing, implementing like other methods(from Utils) are being used.
I don't think checking internet connection at startup is a nice idea. Consider a scenario, where no internet connection is present, and app is launched. What happens then:- User gets notified regarding connection via some UI change right at the launch!, which decreases the UX right from the beginning of the launch, instead, the connection should be checked just before when a request is made that really needs connection rather checking the same beforehand.

@Bhavnaharitsa
Copy link
Contributor

@s-ayush2903 You mean, we'd be using or interfacing the singleton class wherever the internet connectivities are required? If that's the case, then we can use that in the Splash screen to check if the user has a valid connection to proceed to the SignIn process because right after the SplashScreen comes the SignIn page which I guess requires a good internet connection.

No, not interfacing, implementing like other methods(from Utils) are being used.
I don't think checking internet connection at startup is a nice idea. Consider a scenario, where no internet connection is present, and app is launched. What happens then:- User gets notified regarding connection via some UI change right at the launch!, which decreases the UX right from the beginning of the launch, instead, the connection should be checked just before when a request is made that really needs connection rather checking the same beforehand.

@s-ayush2903 But what is needed is needed. The user needs an internet connection to sign in via google. But like you said, the internet connection varies from point to point. The user might have internet connection for now and suddenly something happens and he is out of internet. I personally suggested the permissions in the beginning for certain permissions like Camers which we have in QR code scanning and certain other stuff namely doesnt come to my mind.

@shiv07tiwari
Copy link
Member

No adding a connectivity check only at the beginning isn't a good idea. I was just wondering if we might require some actions to be done alongside the splash screen, now or in the future. So isn't it better if we go with the timer method?

@devansh-299
Copy link
Collaborator Author

No adding a connectivity check only at the beginning isn't a good idea. I was just wondering if we might require some actions to be done alongside the splash screen, now or in the future. So isn't it better if we go with the timer method?

The timer approach will be better if we use it for actions like asking permissions or doing any other
background processing. So should I update the PR and add required alongside actions?

import android.os.Bundle;
import org.mifos.mobilewallet.mifospay.auth.ui.LoginActivity;

public class SplashScreenActivity extends AppCompatActivity {

Choose a reason for hiding this comment

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

Splash screens are meant to engage users by the time app is launched and prevent showing a white screen in older devices while the OS starts the first activity and it should be very quick. If we don’t want to force our app users to wait when there is nothing to load -which is not recommended doing it, then there is no need to create a different activity for the Splash Screen. We can implement it with a lesser line of code. Please take a look at this PR or go through this article.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Splash screens are meant to engage users by the time app is launched and prevent showing a white screen in older devices while the OS starts the first activity and it should be very quick. If we don’t want to force our app users to wait when there is nothing to load -which is not recommended doing it, then there is no need to create a different activity for the Splash Screen. We can implement it with a lesser line of code. Please take a look at this PR or go through this article.

Thanks you very much for the suggestion, I did read this article before. The current approach does handle the ColdStart's white screen but after going through other articles also, most of them suggested to use a separate activity. It does increase the code but it, in my opinion, this increases the code clarity. Thanks, if needed I shall update the PR :)

Choose a reason for hiding this comment

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

Ook, that is nice :). Actually I don't know why most of them suggest a separate activity if we don't need any background processing, although, activities are heavy and expensive operation for OS and it should be used only if we need an independent screen, as you know even Google's recommended approach for app development is a single activity approach. So if you think it really necessary to create an entirely different activity for showing a logo to the screen because of increasing the code clarity then that is a valid point too :)

@jawidMuhammadi
Copy link

No adding a connectivity check only at the beginning isn't a good idea. I was just wondering if we might require some actions to be done alongside the splash screen, now or in the future. So isn't it better if we go with the timer method?

The timer approach will be better if we use it for actions like asking permissions or doing any other
background processing. So should I update the PR and add required alongside actions?

I think it is much better to ask permission whenever we need it, like when the user wants to open a camera , etc... so they will know why we are asking this permission. And what do you mean by background processing, like what?

@devansh-299
Copy link
Collaborator Author

No adding a connectivity check only at the beginning isn't a good idea. I was just wondering if we might require some actions to be done alongside the splash screen, now or in the future. So isn't it better if we go with the timer method?

The timer approach will be better if we use it for actions like asking permissions or doing any other
background processing. So should I update the PR and add required alongside actions?

I think it is much better to ask permission whenever we need it, like when the user wants to open a camera , etc... so they will know why we are asking this permission. And what do you mean by background processing, like what?

I just proposed that if we use timer approach, then we can use that time for any required processing that we may need now or in the future.

@devansh-299
Copy link
Collaborator Author

@shiv07tiwari I have updated the PR as per discussion

Copy link
Member

@shiv07tiwari shiv07tiwari left a comment

Choose a reason for hiding this comment

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

LGTM

@shiv07tiwari shiv07tiwari merged commit a2848fd into openMF:dev May 20, 2020
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.

feat: Intro Splash Screen
5 participants