Closed thomashorta closed 3 weeks ago
App Name | Jetpack | |
Flavor | Jalapeno | |
Build Type | Debug | |
Version | pr20640-18a893e | |
Commit | 18a893ee565ba7d38ae5e4f67e9ef1f4c982db5a | |
Direct Download | jetpack-prototype-build-pr20640-18a893e.apk |
App Name | WordPress | |
Flavor | Jalapeno | |
Build Type | Debug | |
Version | pr20640-18a893e | |
Commit | 18a893ee565ba7d38ae5e4f67e9ef1f4c982db5a | |
Direct Download | wordpress-prototype-build-pr20640-18a893e.apk |
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 40.49%. Comparing base (
42220a6
) to head (18a893e
). Report is 166 commits behind head on trunk.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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.
I installed the build to test this but I cant see the behaviour, sorry.
Could you share the device and Android version you tried? I used an emulator with Android 14 while developing and it works as expected.
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
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.
Nice! I will try it and apply the other suggestion you commented and push the changes soon! Thanks for the Review!
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code
Jetpack Pre-Alpha vpr2051-4cd9168 (1418)
@osullivanchris this looks to be the problem. You are not using the build generated by this PR (which should have pr20640
in the filename). A second confirmation is that the version you're using is based on 1418
while the version in this PR is based on 1421
.
Download and install the build for this PR here: https://github.com/wordpress-mobile/WordPress-Android/pull/20640#issuecomment-2057916200
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.txt
if necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):
N/A