-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[RNMobile] Add scrollable screens to the bottom-sheet #26204
Conversation
Size Change: +568 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
Hello @dratwas! From the code perspective it looks properly, but I was testing the PR on Android and discovered one regression in slider spacing:
I've noticed also wrong behavior in android back button functionality, however I'm able to reproduce it on master as well and so prolly it should be handled on the separate PR - android hardware back button should move user back to the previous sheet instead of to the main |
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 good! I tested the flows in the description via Pixel 3a, and everything is working as expected. Nice work!
I left a minor suggestion for a variable rename, but it is not a blocker.
Edit
Oh, just saw Luke's comment (I guess my browser tab was not refreshed). Let's see about resolving the spacing regression before merging.
packages/block-editor/src/components/block-settings/container.native.js
Outdated
Show resolved
Hide resolved
Hey, @lukewalczak I fixed the spacing in the slider component. Could you please check it one more time? :) |
packages/components/src/mobile/bottom-sheet/bottom-sheet-navigation/navigation-screen.native.js
Outdated
Show resolved
Hide resolved
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.
Checked again spacing on Android and right now it looks properly 🎉 Left some minor comments - not a blocker.
GB-mobile PR: wordpress-mobile/gutenberg-mobile#2741
Motivation:
Since we have navigation inside the bottom-sheet, there is a possibility to have a flat list on some of the screens. In that scenario, we need to remove the scroll view from the bottom-sheet dynamically while switching the screen which generates some issues.
In this PR I moved the ScrollView to the navigation-screen and we can set
isScrollable
prop for each screen. I left the ScrollView only for bottom-sheets w/o navigation inside.I added the
isScrollable
prop to theBottomSheet.NavigationScreen
which determines if the content of the screen has FlatList/List/ScrollView inside.How has this been tested?
This PR shouldn't bring any changes in the way the editor works - it is only a different implementation.
Checklist: