RN: Add Bottom Sheet Select Control component #28543
Conversation
enejb
changed the title
|
Size Change: +3.79 kB (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
enejb
added
[Package] Components
packages/block-editor/src/components/inspector-controls-child/index.native.js
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/bottom-sheet-select-control/README.md
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/bottom-sheet-select-control/index.native.js
Show resolved
Hide resolved
packages/block-editor/src/components/block-settings/container.native.js
Outdated
Show resolved
Hide resolved
|
@enejb This looks fantastic, nice work! One tiny thing I noticed is that the margins on the left/right sides of the sub-sheet look a bit too narrow. I believe 16px is our standard margin on BottomSheets. Here's a visual for reference:
|
|
I updated the padding as well. I didn't update the back button mostly since I am using the current componet that is shared with other parts of the app. the spacing is the same as the Colour Picker see. |
|
@enejb This looks great, thanks for tidying that spacing up.
That's fine, the offset on the back button is intentional to match the system-style back button on iOS. |
packages/block-editor/src/components/inspector-controls-sub-sheet/README.md
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inspector-controls-sub-sheet/README.md
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inspector-controls-sub-sheet/README.md
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/bottom-sheet-select-control/index.native.js
Show resolved
Hide resolved
packages/block-editor/src/components/inspector-controls-sub-sheet/README.md
Outdated
Show resolved
Hide resolved
enejb
force-pushed
the
RN/add/bottom-sheet-child-page
branch
2 times, most recently
from
7b4649d
to
9ca4cfd
Compare
There was a problem hiding this comment.
I think this is getting into good shape. Ideally, IMO, the select control and the generic subsheet implementation would be introduced in separate PRs, since the abstractions introduced are for separate concerns.
Select Control
I like the interface, and especially that it encapsulates the Cell, which is a bit cumbersome to use directly. It is implemented very close to how I'd envisioned it. One thing I'd consider adding is an option title prop, specified after label, so its default value could be label. This would make it slightly more flexible in that the button label wouldn't be tied to the header title of the subsheet.
BottomSheet.SubSheet
I've struggled to find an optimal solution for this abstraction. In its current form, it feels to me like the consumers bear too much of the responsibillity of both state management and rendering, yet I don't see a trivial way of avoiding that without a great loss in generality in the potential usage patterns.
If we succeeded in finding a way around that, we still have a limitation regarding nested usage, since we'd encounter route-name collisions. I think, in any case, that limitation should be documented, but I'm concerned it may not always be clear where this assumption is violated. For example, I was considering a lint-rule, but quickly realized that any descendent component would need to know whether any of its parent components were rendered as a child of the sub-sheet (or vice-versa), which could easily be in another file, and potentially even dynamic (platform variants come to mind).
The alternative is a bit verbose, though, in that new screens would need to be declared in the block settings bottomsheet for each component. This seems a natural consequence of our shared usage of the bottomsheet for all blocks' settings, but passing the route-params back from a shared subscreen is of limited use if we are utilizing the component for multiple separate settings in the parent (it likely makes more sense to use a callback instead). We've been able to use the route-params so far, since the screens defined only ever have singular use for a given modal render.
I think what inherently makes this challenging is that we are using a few different (but similar) patterns to address some cross-cutting concerns within the bottomsheet and block settings (namely contexts, slot-fill, and navigation), and there is some overlap in their potential responsibilities. Navigation, for example, is responsible for whether or not a particular screen is rendered via routes and its stack, while the slot-fills used here have a boolean state variable controlling whether they should render. Another area of overlap is the provider / consumers which can pass props "through" the component heirarchy, while navigation uses params to pass similar information (though the screens themselves are conceptually decoupled from a heirarchy, and are only incidentally heirarchial based on how they are routed, or from their stateful relationships within the stack).
In search of a general solution, I think it's worth considering the case for nested subsheets, and perhaps we could define a convention for dynamic route names that serve this purpose. I haven't had a chance to explore this fully yet, but I believe it's possible, via something like this snack proof of concept I've created: https://snack.expo.io/d6RZX2pY9. I think if we get it right, this could allow us to simplify / DRY up a lot of the current code. Wdyt about the pattern used in that snack?
| } | ||
| showSheet={ showSubSheet } | ||
| > | ||
| <SafeAreaView> |
There was a problem hiding this comment.
I wonder whether it makes sense to move SafeAreaView to the subsheet? One less responsibility for the consumers. The only bottomsheet usage I'm aware of is the fullscreen subsheet style = {height: 100%}, and that style could also be conditionally applied at the subsheet level, since we have the prop there.
| > | ||
| <SafeAreaView> | ||
| <NavigationHeader | ||
| screen={ label } |
There was a problem hiding this comment.
Since we are refactoring a bit in this PR, I think a better name for the screen prop is title, especially since the screen in the generic subsheet will have a fixed name now, and this really represents a user-readable string, wherease a screen's name is being used as a programmatic identifier in navigation routes.
There was a problem hiding this comment.
Good idea. I think we should tackle this in a different PR mostly since this one is already getting pretty big and it will require changing a few different components.
packages/components/src/mobile/bottom-sheet/sub-sheet/index.native.js
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/bottom-sheet/sub-sheet/README.md
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-settings/container.native.js
Outdated
Show resolved
Hide resolved
enejb
force-pushed
the
RN/add/bottom-sheet-child-page
branch
from
a30c96e
to
45829be
Compare
| screen={ label } | ||
| leftButtonOnPress={ goBack } | ||
| /> | ||
| <View paddingHorizontal={ 16 }> |
There was a problem hiding this comment.
Can we use style instead? And use the margins from $block-edge-to-content?
|
Thank you for the changes @enejb! I gave this another test and the other issues were fixed but I just found another one on larger screens and the full-width option:
In this case, the Here's the HTML code
|
|
Good catch will work on this soon. |
|
Hey @enejb 👋 I started to test the latest changes but I encountered this issue:
Just by setting the first Here's the HTML
|
enejb
force-pushed
the
RN/add/bottom-sheet-child-page
branch
from
11f73a6
to
5e5cf75
Compare
enejb
force-pushed
the
RN/add/bottom-sheet-child-page
branch
from
5e5cf75
to
65ff7c5
Compare
|
@geriux Thanks for testing. I think I am getting much closer now. TabletSee Full Width alignment selected. Wide Width alignment selected. No Width alignment selected. MobileNo Width alignment selected. No Width alignment selected. Can you take another look? |
|
I tested this on the Android side and iOS (iPhone 11 Pro and iPad Simulator) it worked as expected. I think this is ready to ship! |
Co-authored-by: Gerardo Pacheco <gerardo.pacheco@automattic.com>
Co-authored-by: Gerardo Pacheco <gerardo.pacheco@automattic.com>
enejb
force-pushed
the
RN/add/bottom-sheet-child-page
branch
from
66c2115
to
9724732
Compare
enejb
changed the title
This PR adds a new Bottom Sheet Selector Component and implements it for the Button Block so that you can pick different buttons widths.
Description
This PR does a few different things.
BottomSheetSubSheetcomponent. That is suppose to make it easier to create new controls that navigate using in the BottomSheet one level deep.BottomSheetSelectControlthat lets you pass in bunch of option and lets the user select the oneBottomSheetSelectControlto the Button Settings so that the user can choose the Button Width.How has this been tested?
Load this PR.
Add a button block.
Change the button width.
Does it work as expected.
Screenshots
new-selector.mov
iOS
Types of changes
Checklist: