-
Notifications
You must be signed in to change notification settings - Fork 6
[#102] [Integrate] As a user, I can see dialog when there is no internet connection #105
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
base: develop
Are you sure you want to change the base?
Conversation
CoroutineTemplate Jacoco report:Generated by 🚫 Danger |
@kaungkhantsoe Did you face this issue on your side? When clicking on the Screen.Recording.2566-08-03.at.10.02.43.mov |
app/src/main/java/co/nimblehq/compose/crypto/ui/screens/home/HomeScreen.kt
Outdated
Show resolved
Hide resolved
e0e0e42
to
2dd9657
Compare
61b6af5
to
f031f35
Compare
@Wadeewee Ahh yes, I was trying out a callback function. I will remove it from the |
app/src/main/java/co/nimblehq/compose/crypto/ui/navigation/AppNavigation.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small naming issues. The rest LGTM.
app/src/main/java/co/nimblehq/compose/crypto/ui/navigation/ComposeCryptoApp.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/co/nimblehq/compose/crypto/ui/navigation/ComposeCryptoApp.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/co/nimblehq/compose/crypto/ui/navigation/ComposeCryptoApp.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just minor issues, the rest LGTM now 👌
app/src/main/java/co/nimblehq/compose/crypto/ui/navigation/AppNavigation.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/co/nimblehq/compose/crypto/ui/navigation/AppNavigation.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/co/nimblehq/compose/crypto/ui/navigation/ComposeCryptoApp.kt
Outdated
Show resolved
Hide resolved
d68a173
to
f961999
Compare
@hoangnguyen92dn I have updated the method of showing global dialog with different actions. Please help me review it and let me know if you have any concerns with it. 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the CI failed
title: String, | ||
message: String, | ||
actionText: String, | ||
actions: List<DialogActionModel>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In our case, I think we only need a dialog with 2 buttons 🤔 So it could be positiveButton, negativeButton
, passing the list of model quite confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, sometimes we might need only Ok
button, sometimes we might want one negative
and one positive
buttons, and sometimes we might need other buttons. By passing actions, we can customize the action buttons to whatever we want. What do you think?
…solve minor review comments
f961999
to
a3f5d4b
Compare
What happened 👀
When there is no internet connection, it shows no internet connection dialog.
Insight 📝
There is an issue in using ConnectivityManager.NetworkCallback. It doesn't recognize there is no internet at the start of an application in the following scenario
There is a possible workaround but it is out of the scope for this initiative.
Proof Of Work 📹
Screen.Recording.2023-08-25.at.2.33.47.PM.mp4