feat: #678 - added bottom navigation bar to product page #679
Conversation
The previous bottom navigation bottom bar was refactored, in order to be reused anywhere. Then it was added to the product page. Deleted files: * `home_page.dart` * `smooth_ui_library/smooth_bottom_navigation_bar.dart` * `smooth_ui_library/smooth_bottom_navigation_bar_item.dart` New file: * `smooth_bottom_navigation_bar.dart` Impacted files: * `continuous_scan_page.dart`: added an explicit bottom navigation bar (was implicit through `HomePage`) * `main.dart`: not using `HomePage` anymore; asking the bottom navigation bar who's the current page instead * `new_product_page.dart`: added an explicit bottom navigation bar (was not there) * `product_list_page.dart`: added an explicit bottom navigation bar (was implicit through `HomePage`) * `product_page.dart`: added an explicit bottom navigation bar (was not there) * `smooth_ui_library.dart`: removed "UI" version of smooth bottom navigation bar * `user_preferences.dart`: added a preference for tab index * `user_preferences_page.dart`: added an explicit bottom navigation bar (was implicit through `HomePage`)
| Future<void> setBottomTabIndex(final int index) async => | ||
| _sharedPreferences.setInt(_TAG_BOTTOM_TAB_INDEX, index); |
There was a problem hiding this comment.
I haven't read through the whole code yet, but I don't see the value in storing the index. Especially now where besides the scanner the other two pages don't provide that much value anymore.
There was a problem hiding this comment.
@M123-dev The point of storing the index is to use exactly the same navigation bar in all cases: the legacy cases (the former home page, divided in 3 tabs) and the new cases (purpose of this issue, and the product page is dealt with in this PR). And without too much complex code: just add bottomNavigationBar: SmoothBottomNavigationBar() to the Scaffold.
There was a problem hiding this comment.
Hey this PR is pretty close to my solution for this as well, so it looks good to me.
I don't really see why we need the current index though? Is it so if a user closes the app and comes back, they land on the tab they we on when they closed the app ? In that case, it was discussed that whenever a user opens the app they should see the scan page with "Welcome to Open food facts" text on the search box.
There was a problem hiding this comment.
it was discussed that whenever a user opens the app they should see the scan page with "Welcome to Open food facts" text on the search box
Oh, I didn't know that.
Behind saving the current index, there was also the question (already asked): which tab should look "selected" when we're not on one tab page in particular - e.g. when we are on the product page?
There was a problem hiding this comment.
So in a nutshell, this is what I understand the behavior to be:
- Navigation bar becomes sticky
- If the user goes into a tab and clicks around (example: opens the product page from history tab), he is still in that tab, just browsing
- Tabs are sticky, so lets say a user is on the "Product page" from the "History tab", he clicks on "Preferences tab", then clicks back on the "History tab", he will land on the "Product page" he was recently viewing, rather than on the "history page". Tapping again on the History tab will take the user to the "History page". This behavior is akin to a few other apps like Gmail/parts of GMaps etc.
- When the user opens the app, the first tab displayed to the user is always the Scan page.
There was a problem hiding this comment.
Navigation bar becomes sticky
More precisely, navigation bar was added to the product page. I'm not sure if there are other pages beyond profile, scan, history and product, but if so, the navigation bar should be added there too.
If the user goes into a tab and clicks around (example: opens the product page from history tab), he is still in that tab, just browsing
No, because you asked me to remove the tab index from the preferences and that's where I kept its value. If you go to a product page, the navigation bar will display the default tab as selected: the scan tab. Anyway I didn't get the answer to "which tab should be selected when we are on the product page", and in the meanwhile I decided to select the scan tab.
Tabs are sticky, so lets say a user is on the "Product page" from the "History tab", he clicks on "Preferences tab", then clicks back on the "History tab", he will land on the "Product page" he was recently viewing, rather than on the "history page". Tapping again on the History tab will take the user to the "History page". This behavior is akin to a few other apps like Gmail/parts of GMaps etc.
No, if you click on the history tab you always go to the history page.
This PR is only about issue #678, and does not implement the not-even-existing-yet "Make tabs stateful" issue evoked in #655.
When the user opens the app, the first tab displayed to the user is always the Scan page.
That's correct, the default tab (which is opened in main.dart) is the scan tab.
There was a problem hiding this comment.
Hey, my previous message was more around generally what the navigation behavior should be like and not about how it would be after this PR. Sorry for misunderstanding. This PR adds the feature it's meant to add, so thank you for that.
I'm not sure if there are other pages beyond profile, scan, history and product, but if so, the navigation bar should be added there too.
There's also the settings page, but that one is not that important, also doesn't feel like we should make the settings page sticky. Let's skip it for now. If we have to add the bar to it later, we can do so.
"which tab should be selected when we are on the product page"
So if the user goes to a tab (lets say history tab) and clicks around on the page and lands on some other view (lets say product page), he is considered to still be on the history tab, so the history tab should appear as selected. I was thinking this should be doable without storing the tab index in user prefs. if we instantiate with bottomNavigationBar: const SmoothBottomNavigationBar(tab=SmoothBottomNavigationTab.History) in new_product_page
Impacted files: * `continuous_scan_page.dart`: explicitly select the "scan" tab * `new_product_page.dart`: implicitly select the "default" tab * `product_list_page.dart`: explicitly select the "history" tab * `product_page.dart`: implicitly select the "default" tab * `smooth_bottom_navigation_bar.dart`: now explicitly selecting the tab, instead of storing it in preferences * `user_preferences.dart`: removed tab index preference * `user_preferences_page.dart`: explicitly select the "profile" tab
Impacted file: * `main.dart`: implicitly select the "default" tab and the matching page
| ), | ||
| }; | ||
|
|
||
| static Widget getCurrentPage({ |
There was a problem hiding this comment.
do we only need to expose this method outside? it's only used in main.dart, maybe we can call it getDefaultPage
|
Thank you @jasmeet0817 for the code review. I guess the next step is about "Make tabs stateful". I'm going to create the issue, but I will probably not code it. |
The previous bottom navigation bottom bar was refactored, in order to be reused anywhere.
Then it was added to the product page.
Deleted files:
home_page.dartsmooth_ui_library/smooth_bottom_navigation_bar.dartsmooth_ui_library/smooth_bottom_navigation_bar_item.dartNew file:
smooth_bottom_navigation_bar.dartImpacted files:
continuous_scan_page.dart: added an explicit bottom navigation bar (was implicit throughHomePage)main.dart: not usingHomePageanymore; asking the bottom navigation bar who's the current page insteadnew_product_page.dart: added an explicit bottom navigation bar (was not there)product_list_page.dart: added an explicit bottom navigation bar (was implicit throughHomePage)product_page.dart: added an explicit bottom navigation bar (was not there)smooth_ui_library.dart: removed "UI" version of smooth bottom navigation baruser_preferences.dart: added a preference for tab indexuser_preferences_page.dart: added an explicit bottom navigation bar (was implicit throughHomePage)