[Reader Customization] Allow dragging to close Preferences bottom sheet #20640
Conversation
It still doesn't save the user selection when dismissed via dragging.
thomashorta
requested review from
notandyvee,
RenanLukas,
develric and
osullivanchris
|
 |
App Name |  Jetpack |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr20640-18a893e | |
| Commit | 18a893e | |
| Direct Download | jetpack-prototype-build-pr20640-18a893e.apk |
|
 |
App Name |  WordPress |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr20640-18a893e | |
| Commit | 18a893e | |
| Direct Download | wordpress-prototype-build-pr20640-18a893e.apk |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #20640 +/- ##
=======================================
Coverage 40.48% 40.49%
=======================================
Files 1479 1479
Lines 68342 68349 +7
Branches 11290 11290
=======================================
+ Hits 27669 27676 +7
Misses 38173 38173
Partials 2500 2500 ☔ View full report in Codecov by Sentry. |
|
I installed the build to test this but I cant see the behaviour, sorry. In theory scrolling to close screens is something I'm generally in favour of. Though I am interested in how it affects or doesn't the presentation of the screen. Should a screen come from above rather than from the left if it can be dragged down. And similarly should it have an X icon like a modal view rather than a back arrow. |
Could you share the device and Android version you tried? I used an emulator with Android 14 while developing and it works as expected. reader-customization-draggable.mp4 |
Looks pretty good in the video. To add to my other question, I wonder should we be doing this in many places, not just one |
There was a problem hiding this comment.
Overall, great job. Don't have much to say other than to utilize kotlin styling a bit more and remove some curlies. But I guess you can argue its a nit 😅 .
The main caveat is that dragging down conflicts with preview content scroll when the font is too large and/or the screen size is too small, which has the potential to be more annoying than dragging being disabled, so it would be great to have feedback from the Reviewers here and maybe merging this change and keeping an eye out for user complaints/reviews.
I'm familiar with the general problem. Never used compose though. So took the opportunity to test and play around with it. In ReadingPreferenceScreen, the very first Column, add
Column(
modifier = Modifier.fillMaxSize().nestedScroll(rememberNestedScrollInteropConnection()),
This nestedScroll will tell the bottom sheet's coordinatorlayout to work with nested scrollable containers. Works for me. I can't guarantee this is the best approach though since I havent dealt with compose much yet. Hopefully this helps.
| * The bottom sheet has been hidden by the user, which means the dismiss process is already on its way. All we need | ||
| * to do is save the current preferences. | ||
| */ | ||
| fun onBottomSheetHidden() { |
There was a problem hiding this comment.
You can remove the function curly braces = launch.
There was a problem hiding this comment.
I'm not a big fan of that approach since doing that causes the function to return a Job, which is not really what I'm going for and it would be just a side-effect of trying to make the code look prettier.
I get that using = launch is kinda common practice. Still, I personally think that the function signature should better match the intention of the code, so if no callers need the returned Job (and they shouldn't as this function is public and the coroutine is an implementation detail relevant to the ViewModel only) I prefer "returning" Unit.
...rc/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderReadingPreferencesViewModel.kt
Outdated
Show resolved
Hide resolved
| * An exit action has been triggered by the user. This means that we need to save the current preferences and emit | ||
| * the close event, so the dialog is dismissed. | ||
| */ | ||
| fun onExitActionClick() { |
There was a problem hiding this comment.
Ditto here. Just to save some curly braces.
There was a problem hiding this comment.
Same reasoning for not returning Job here.
Nice! I will try it and apply the other suggestion you commented and push the changes soon! Thanks for the Review! |
|
@osullivanchris this looks to be the problem. You are not using the build generated by this PR (which should have Download and install the build for this PR here: #20640 (comment) I suggest waiting a few minutes though since I just pushed the changes fixing the nested scroll that @notandyvee suggested (thanks again btw!), so a new build should be up soon. |
Improvement suggested by @notandyvee here: pcdRpT-6gD-p2#comment-9800
Allow dragging to hide and close the Reading Preferences bottom sheet, saving the latest user selection.
The main caveat is that dragging down conflicts with preview content scroll when the font is too large and/or the screen size is too small, which has the potential to be more annoying than dragging being disabled, so it would be great to have feedback from the Reviewers here and maybe merging this change and keeping an eye out for user complaints/reviews.
If anyone knows if there's some sort of way of providing some "scroll preference order" to the gesture system so it first scrolls the content and only scrolls the sheet when content is at the top, that would be ideal, but I couldn't find something like that. 😞
To Test:
Make sure to also test using large font sizes and trying to drag the preview content.
Note: It's possible to drag the content consistently if the user starts by dragging the finger UP before trying to drag it down.
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR Submission Checklist:
RELEASE-NOTES.txtif necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):
N/A