-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Rework the app bar #6320
Rework the app bar #6320
Conversation
@alyblenkin I've reworked the app bar in the main menu so that the behavior is consistent with https://m3.material.io/components/top-app-bar/guidelines#9410573d-05c7-473e-8810-5deea97b9b64 |
It will take some getting used to, but I think this will be easier for us to maintain if it's consistent with M3 styling. Part of me is thinking maybe we always display the tonal colour (not just when you scroll), but it's hard to say without seeing it applied throughout the app. It might be easier to make that call once we apply it to another screen, like selecting a new form. |
0bad2d0
to
8ae288b
Compare
@alyblenkin ok the list of blank forms is reworked as well so you can test it. |
Great, thank you! I'm liking it more today. I think we should go ahead and refactor the other screens. I noticed it looks a little strange with our grey banner for forms that are ready to send. I think we will want to make the banner background white, so that entire section if fixed when you scroll. |
8ae288b
to
ca50a83
Compare
ca50a83
to
26f5dfb
Compare
@alyblenkin now the whole app is reworked. If you want to take another look now it's a good time. |
3f9e4ca
to
e2f0fa6
Compare
It will still take some getting used to, but it looks good. I think one thing we will want to change is the ready to send page. I think we should remove the grey banner because it looks strange without the app bar elevation. Do you want to do that as part of this work or as a new issue? |
Please file a separate issue. |
Yes the M3 doc says the easiest way is to enable displaying content edge-to-edge: https://github.com/material-components/material-components-android/blob/master/docs/components/TopAppBar.md#status-bar-and-edge-to-edge
@alyblenkin do you think that's what we should do here? cc @seadowg |
This is already part of #6146, but I think it's worth seeing if it's a quick change and then incorporating here if so. If not, we can live with the in between stage for now. |
It's not big but kind of annoying as we would need to enable it in every activity: https://developer.android.com/develop/ui/views/layout/edge-to-edge#enable-edge-to-edge-display. It might also require some additional changes https://developer.android.com/develop/ui/views/layout/edge-to-edge#handle-overlaps I think it would be better to have it in a separate pull request. |
Agreed! It's indeed a really annoying change. |
I guess it's because the search field does not take the whole space vertically and the empty space below is still a part of the app bar but I will check that.
Yes this is how app bars should look like in M3 https://m3.material.io/components/top-app-bar/guidelines#9410573d-05c7-473e-8810-5deea97b9b64 |
I'm not sure also about this one. When I try to scroll down when an audio is being played/recorded, the app bar flashes. I guess it as if the screen was "refreshed" very quickly. Should I file an issue and investigate this a bit more or it's ok? XRecorder_26082024_125935.mp4 |
It shouldn't be like that so yes you can file an issue. |
Tested with Success! Verified on a device with Android 10 Verified Cases:
|
Tested with Success! Verified on a device with Android 14 |
Closes #6135
Why is this the best possible solution? Were any other approaches considered?
We agreed that we should start using the Material 3 app bar. We might need to adjust colors later on but for now it should be enough. Before we used different app bars in different places now it's one app bar layout used everywhere so it should look and behave in the same way.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
We need to test the app bars across the entire app to ensure they are consistent and visually appealing. Nearly every screen, except for map views, includes an app bar. Please also remember to test full-screen dialogs that contain app bars, including:
It’s important to test scrolling on each of these screens. If the screen isn’t long enough to scroll, you can:
Do whatever is necessary to ensure the screen is scrollable.
There’s a risk that some of the screens with app bars may not look the same as before. I had to rework their layouts, so it’s possible that I made a mistake somewhere please be careful.
There's one issue that concerns me. I had to remove this line: https://github.com/getodk/collect/pull/6320/files#diff-345fc063e7a817a7c0a68f814d7caf8ddafa8709c69474945fad7ba9f00a33b3L84 to enable the app bar to change color while scrolling. It appears that the line was redundant, which I attempted to verify using this form:
select_one_fieldlist.xlsx.
In general, this relates to scrolling through a list of choices. The list should contain no more than 40 elements and ideally, there should be some other questions below. Please ensure that scrolling through such lists works as it did before.
Do we need any specific form for testing your changes? If so, please attach one.
To test
User identity
andReason for changes
dialog I used:audit.xlsx
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest
(or./gradlew testLab
) and confirmed all checks still passDateFormatsTest