-
Notifications
You must be signed in to change notification settings - Fork 8
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
[1795] Add status view about internet connection #1839
base: master
Are you sure you want to change the base?
Conversation
Hi! Please make sure to sign the CLA, add some testing and use |
Please @WentuM sign the CLA if you want your PR to be reviewed and merged |
d741d90
to
9b0ac6f
Compare
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.
I'd also like not only to test with unity tests the connectivity flag, it would be nice to also test the correct UI behaviour with an integration test (i.e. when connection is down the banner pops up correctly).
Also, please include in the PR description screenshot of this 'no connection' alert.
import javax.inject.Singleton | ||
|
||
@Singleton | ||
class ConnectivityRepository @Inject constructor(application: Application) { |
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.
I don't think there's the need for a connectivity repository, as repositories have a different purpose than this. I'd suggest putting this in an utility class as in the end it's just a static method to which you can pass the application context
@@ -56,6 +60,8 @@ constructor( | |||
/** This LiveData boolean is used to indicate whether the HomeFragment is displayed */ | |||
val isHome = MutableLiveData(java.lang.Boolean.TRUE) | |||
|
|||
val isInternetConnected = MutableLiveData(java.lang.Boolean.TRUE) |
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.
Perhaps I'd refactor this to isConnectedToInternet
viewModel.observeInternetConnection() | ||
|
||
handleTopAppBar() | ||
|
||
observeInternetConnection() | ||
|
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.
I'd avoid to have 2 function calls semantically so close, so you can call that viewModel.observeInternetConnection
in the observeInternetConnection
method
Added a status view to the android version of the app with a subscription to the current internet connection