-
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
Sort menu redesign - Bottom Sheet Dialog #1242
Sort menu redesign - Bottom Sheet Dialog #1242
Conversation
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.
This looks and works really well for me in portrait! It disappears on screen rotation, though, and doesn't show all the way in landscape. This is on a Galaxy Tab A, Android 5.1.1.
@@ -28,6 +31,24 @@ | |||
"sq", "sw", "sw_KE", "ta", "th_TH", "tl", "tr", "uk", "ur", | |||
"ur_PK", "vi", "zh", "zu"}; | |||
|
|||
public static int getSortIcon(String sortBy) { |
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.
This constants class should really only hold true constants and no logic.
fc1898c
to
f2597b3
Compare
@lognaturel Please review |
Nice, @shobhitagarwal1612! For Edit Saved Form, there are a lot of options and so some scrolling is needed to see all of them. That wasn't immediately obvious to me. Is there some standard way of identifying that scrolling may be needed? @mmarciniak90 can you please do a test pass? @grzesiek2010 would be great to get your eyes on it as well and see what you think. I also want to do another code review pass. |
I need to do another thorough code review.
I noticed problems. Tested on Android 4.1, 4.2, 4.4, 5.1.
Other similar case:
Those problems occur only on sort list with 2 options |
190d37e
to
db805ea
Compare
@mmarciniak90 @lognaturel Please review |
@grzesiek2010 @mmarciniak90 @yanokwa I think this is really nice and I think it's pretty unlikely to confuse users. What would you think about merging it to |
Samsung Galaxy Duos
I see this crash if i open the sort menu and change the device orientation. it works well only in I'm not able to reproduce this issue on Samsung Galaxy S4 Android 5.0.1 |
@grzesiek2010 Fixed |
ok I'll look at it again. |
4fe9130
to
ddaf9cf
Compare
@lognaturel I have switched the base branch to master |
20d1c3b
to
656e0ca
Compare
The crash doesn't occur but the problem with blue icons is still visible. |
@grzesiek2010 In which screen did you notice this? |
each one |
Omits `by` from the sort options labels
656e0ca
to
5165133
Compare
@grzesiek2010 I am unable to reproduce the bug. I am testing on API 25 emulator. Can you please describe the steps to reproduce this bug? |
There aren't any special steps just:
The same if you use Date options. Everything is ok if you sort by status because there are two different icons I guess. |
@grzesiek2010 Fixed. Please test again. |
@shobhitagarwal1612 @lognaturel I can confirm that both issues I found have been fixed. @mmarciniak90 will continue testing when she is free. |
@shobhitagarwal1612 If you fix these merge conflicts one more time, this is ready to go in. 🎉 |
# Conflicts: # collect_app/src/main/res/values-hi/strings.xml # collect_app/src/main/res/values-sq/strings.xml # collect_app/src/main/res/values-sv-rSE/strings.xml
…e-1039-sortdialog # Conflicts: # collect_app/src/main/res/values-sv-rSE/strings.xml
@lognaturel Merged the conflicts |
Fixes #1039