Offline mode behavior #645
Conversation
|
@veloce If it's possible review pls. |
|
I will review after #644 |
lib/src/navigation.dart
Outdated
| @@ -110,6 +117,8 @@ class BottomNavScaffold extends ConsumerWidget { | |||
| @override | |||
| Widget build(BuildContext context, WidgetRef ref) { | |||
| final currentTab = ref.watch(currentBottomTabProvider); | |||
| final connectivity = ref.watch(connectivityChangesProvider); | |||
| final isOnline = connectivity.value?.isOnline ?? false; | |||
There was a problem hiding this comment.
You should not assume the value is false when in loading state, I'd prefer to have true by default.
Also the connectivity is only needed for the watch tab, so instead of having the ref.watch here, it would be better to wrap the NavigationDestination with a Consumer widget so only that one rebuilds.
There was a problem hiding this comment.
It's impossible cause i need this value in onDestinationSelected cb, all what can i do, it wrap NavigationBar with Consumer, but I coudn’t do it with CupertinoTabBar.
lib/src/navigation.dart
Outdated
| @@ -98,6 +100,11 @@ final watchScrollController = ScrollController(debugLabel: 'WatchScroll'); | |||
| final RouteObserver<PageRoute<void>> rootNavPageRouteObserver = | |||
| RouteObserver<PageRoute<void>>(); | |||
|
|
|||
| final cupertinoTabControllerProvider = | |||
There was a problem hiding this comment.
Not really useful to declare a state provider here, you could just use a private final variable.
| @@ -602,16 +602,21 @@ class _PlayerScreenButton extends ConsumerWidget { | |||
|
|
|||
| @override | |||
| Widget build(BuildContext context, WidgetRef ref) { | |||
| final connectivity = ref.watch(connectivityChangesProvider); | |||
| final isOnline = connectivity.value?.isOnline ?? false; | |||
There was a problem hiding this comment.
You should not access value directly but use AsyncValue's when() method: https://pub.dev/documentation/riverpod/latest/riverpod/AsyncValue-class.html
There was a problem hiding this comment.
If a consumer of an AsyncValue does not care about the loading/error state, consider using value/valueOrNull to read the state:
Do we really need error and load value here?
There was a problem hiding this comment.
TBH I find the async value api a bit confusing. But ok we can use value here. Just I don't like it to be false by default. I'd rather assume the connectivity is on than off when we don't know it yet.
|
Just added a couple of tweak on top of it. Thanks! |
Issue #462
In play tab:
In the bottom navigation bar: