Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Add Tab bar (EXPOSUREAPP-4639) #2235

Merged
merged 40 commits into from
Feb 3, 2021
Merged

Conversation

mtwalli
Copy link
Contributor

@mtwalli mtwalli commented Jan 29, 2021

1- Add Tab in Home Screen
2- Move Contact diary nav graph into main graph as nested graph
3- Remove contact diary Activity
4- Adjust AppTheme to work with AppCompat and MDTheme
5- Remove Diary Card from Home Screen
6- Remove Overview diary screen <- button, Onboarding x button.

Tested Dark and Light Themes on Samsung S8 Android 9

Update: Alignment discussion with Martin regarding the Diary onboarding screen

Q: Hi Martin, I have a question regarding the tab bar changes, does the contact onboarding screen have to show tab bar or not? this screen is also accessible from Information menu in the top, in this case is the same behaviour applied ?

A:Hi, right. We have discussed that we will have two behaviours there. If you open it from the upper menu, it will be shown as a modal dialog and the Tab-Bar will be hidden. In case you enter the contact diary the first time we will display it as a normal view behind the tabs (so that we avoid that I press on the tab and the Tab will be covered by the modal dialog).

PLEASE test it thoroughly and report any difference you spot

@mtwalli mtwalli requested review from a team January 29, 2021 09:58
@mtwalli mtwalli added maintainers Tag pull requests created by maintainers text change PRs with text changes. labels Jan 29, 2021
@mtwalli mtwalli added this to the 1.13.0 milestone Jan 29, 2021
@harambasicluka
Copy link
Contributor

Tested on a Pixel 3, looks really good and feels smooth :)
I'm not a big fan of the "jumping" between the logo and the title, but that's not in our hands.

@mtwalli
Copy link
Contributor Author

mtwalli commented Feb 1, 2021

Only did a on device testing (Pixel 3), due to #2235 (comment) I visited every screen, I wasn't able to spot a difference.

But I spotted a "jumping", I think it isn't caused by this change.
When I go from home to diary everything works as expected but if I navigate to test and go back it seems like the risk card is redrawn, in this video it isn't very visible but it should be easy to reproduce.

device-2021-02-01-172504.mp4

The jumping is happening because we hide the bottom bar for second level screens. it takes some space back once it visible. in other words it pushes the content up

@mtwalli
Copy link
Contributor Author

mtwalli commented Feb 1, 2021

Did extensive device testing with QR-codes of all types, TAN, App reset, went through all settings, tested all features of the contact journal, ... on Pixel (Android 10). Worked as expected and could not detect any UI glitches. Nice job!

The code looks good too. Happy to approve after the VM comment and weak reference comment have been addressed.

I moved the settings to the view model. I will keep the weak reference to have the same behaviour look at my comment ⬆️

ralfgehrer
ralfgehrer previously approved these changes Feb 2, 2021
@LukasLechnerDev
Copy link
Contributor

Tested on Samsung S20+, Android 11. Looks good, basically.

The only thing I found is that the EditTexts in the contact diary now look different and lack the display of remaining characters.

image

@SamuraiKek
Copy link
Contributor

SamuraiKek commented Feb 2, 2021

Tested on Huawei Mate 8 (Android 6). This feels almost perfect. I have also found the issue mentioned by @LukasLechnerDev. Other than that, while I'm on the home screen, if I open the three dot menu, and choose one of the options there, the tab bar disappears before the screen is switched (looks kinda weird).
Also, can we save the state of the activities when switching between tabs? If I scroll down on the home screen and then switch to the diary and then switch back to the home screen, it looks like the whole activity is redrawn and I'm starting from the to top, not from where I left off.

@mtwalli
Copy link
Contributor Author

mtwalli commented Feb 2, 2021

Tested on Samsung S20+, Android 11. Looks good, basically.

The only thing I found is that the EditTexts in the contact diary now look different and lack the display of remaining characters.

image

Thanks for reporting! , this should be addressed by c82cc1f

@mtwalli
Copy link
Contributor Author

mtwalli commented Feb 2, 2021

Tested on Huawei Mate 8 (Android 6). This feels almost perfect. I have also found the issue mentioned by @LukasLechnerDev. Other than that, while I'm on the home screen, if I open the three dot menu, and choose one of the options there, the tab bar disappears before the screen is switched (looks kinda weird).
Also, can we save the state of the activities when switching between tabs? If I scroll down on the home screen and then switch to the diary and then switch back to the home screen, it looks like the whole activity is redrawn and I'm starting from the to top, not from where I left off.

Thx, I will try to improve or (delay) the visibility change part. Regarding resetting the state , I personally don't like too. You would be surprised to know that this the recommended MD behaviour for BottomView on Android :https://material.io/components/bottom-navigation#behavior

@jurajkusnier
Copy link
Contributor

Teste on Pixel 4 (Android 10) and didn't notice any issues

@LukasLechnerDev
Copy link
Contributor

👍 Checked the EditTexts again and they look good now:

image

@sonarcloud
Copy link

sonarcloud bot commented Feb 2, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

4.1% 4.1% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@chiljamgossow chiljamgossow left a comment

Choose a reason for hiding this comment

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

tested on Xiaomi
LGTM

@PhilippNowak96
Copy link
Contributor

PhilippNowak96 commented Feb 22, 2021

@mtwalli sorry for pulling out this old one but I stumbled over it when testing App Shortcuts. Did you guys discuss about suppressing reselection of bottom navigation items? The current behaviour is the fragment is created every time you click the icon, even if it is already the active one. Despite being the default behaviour I'm not sure if it is useful here.

@dsarkar
Copy link
Member

dsarkar commented Feb 22, 2021

@PhilippNowak96 forwarded your comment to the devs ticket.

@mtwalli
Copy link
Contributor Author

mtwalli commented Feb 23, 2021

@mtwalli sorry for pulling out this old one but I stumbled over it when testing App Shortcuts. Did you guys discuss about suppressing reselection of bottom navigation items? The current behaviour is the fragment is created every time you click the icon, even if it is already the active one. Despite being the default behaviour I'm not sure if it is useful here.

@PhilippNowak96, Thanks for the feedback, as you already mentioned this is the default behaviour in the navigation components and the material design guidelines: https://material.io/components/bottom-navigation#behavior.

If you feel that the default behaviour is not providing a good user experience, I suggest that you open a feature request and explain what should be the expected behaviour. So we can better asses your request and discuss it with the UX team.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
author merge PR to be merged by author maintainers Tag pull requests created by maintainers text change PRs with text changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet