[Mobile] Update the bottom sheet header #34309
Conversation
enejb
added
the
Mobile App - i.e. Android or iOS
enejb
changed the title
|
Size Change: -1.95 kB (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
enejb
changed the title
There was a problem hiding this comment.
I tested the following in the demo app for both iOS and Android, including both light and dark mode, and confirm that the header looks visually the same as before the changes:
-
Button Block
- Button Width
- Link Settings
-
Image block
- Alt text
-
Cover Block
- Edit Focal Point
- Color Picker
-
Paragraph Block
- Font Site Settings.
- Colour Settings
I made a couple of small suggestions for the README and also asked a question about the navigation-header-native.js file (i.e. whether there's a reason it can't be deleted). Other than that, all the code makes sense to me and I agree the updated component is more readable. :) As I couldn't spot any obvious big issues or blockers, I've approved from my side.
| @@ -600,6 +601,7 @@ ThemedBottomSheet.Button = Button; | |||
| ThemedBottomSheet.Cell = Cell; | |||
| ThemedBottomSheet.SubSheet = BottomSheetSubSheet; | |||
| ThemedBottomSheet.NavigationHeader = NavigationHeader; | |||
There was a problem hiding this comment.
As far as I can see, the NavigationHeader component is no longer in use anywhere and removing this reference doesn't have any effect on the BottomSheet's header component. Is there a reason for keeping the bottom-sheet/navigation-header.native.js file and references to the component?
There was a problem hiding this comment.
I think we can remove it. Good catch!
| Header components is meant to be used to compose the header inside the BottomSheet. | ||
|
|
||
| ### Usage | ||
| See |
There was a problem hiding this comment.
| See |
It looks like this can either be removed or perhaps there was a missing referencing?
| @@ -0,0 +1,91 @@ | |||
| # Header | |||
|
|
|||
| Header components is meant to be used to compose the header inside the BottomSheet. | |||
There was a problem hiding this comment.
| Header components is meant to be used to compose the header inside the BottomSheet. | |
| `BottomSheet.Header` should be used to add a header to the top of a `BottomSheet` component. It makes several other components available, which can then be used to compose the header's content. |
It took me a bit to fully process how this component works, so think adding a bit more detail to the intro might help with clarity. WDYT?
There was a problem hiding this comment.
💯 This is really great work! Thank you!
I think abstractions like this require a lot of thought to get right. So, thank you for spending the time on this and please have grace with the large amounts of comments questions I pose. 😄 I appreciate you putting this together.
I noticed that the bottom sheet for the Columns block variation selection does not use these components. Is that intentional? I would think we should use these new components there as well for consistency. WDYT?
packages/components/src/mobile/bottom-sheet/header/action-button.native.js
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/bottom-sheet/header/back-button.native.js
Outdated
Show resolved
Hide resolved
| function CloseButton( { onPress } ) { | ||
| return <CancelButton onPress={ onPress } text={ __( 'Close' ) } />; | ||
| } |
There was a problem hiding this comment.
I'm not sure if the small text change to "Close" merits an entire component. I'm not sure it provides value. WDYT about leveraging CancelButton and passing text as a prop when needed instead?
<CancelButton onPress={ () => {} } text={ __( 'Close' ) } />There was a problem hiding this comment.
I am not a big fan of the text prop. Mostly because I would expect the text to be passed down with via the children like
<Button onPress={ () => {} }>{ __( 'Close' ) } ></Button>
but in this case, because we are not just rendering all the children but only in special cases. (only on iOS) having a prop of text could work well and we can make the component a bit more generic. and remove the need for the different components.
Another way we would accomplish the same thing would be if a had a specific some sort of type and then pass in the different ones. ( 'close', 'back', 'close' ) this would also help us set different accessibility hints in the future.
I am not a fan of this approach mostly since it creates type is a bit generic term and we end up in a place where we don't really know all the different types.
There was a problem hiding this comment.
Coming back to think I think once we have more specific accessibility Hint and specific content.
I think it would make sense to have a block for each of the different "BackButtons" s.
Also, the code would read better and we could avoid things like
<BottomSheet.Header.CancelButton onPress={ close } text={ __( 'Close' ) } />
There was a problem hiding this comment.
I don't have strong opinions about the granularity of the components in this case, but I wanted to mention that there may be cases where "Close" "Cancel" and "Back" are use somewhat inconsistently. The componentization in this PR might also be a good opportunity to clarify the intended semantics of these.
There was a problem hiding this comment.
I am not a big fan of the
textprop. Mostly because I would expect the text to be passed down with via the children [...]but in this case, because we are not just rendering all the children but only in special cases. (only on iOS) having a prop of
textcould work well and we can make the component a bit more generic. and remove the need for the different components.
Understandable. I think relying upon children over text generally makes sense and should be the default. In reality, the fact that the text is conditionally rendered doesn't prohibit us from setting it with children. We could still pass it as children.
That said, would it be better to communicate the conditional rendering via the property name? E.g. iosText.
<BottomSheet.Header.CancelButton iosText={ __('Close') } />Another way we would accomplish the same thing would be if a had a specific some sort of
typeand then pass in the different ones. ( 'close', 'back', 'close' ) this would also help us set different accessibility hints in the future.
I am not a fan of this approach mostly since it creates type is a bit generic term and we end up in a place where we don't really know all the different types.
I agree that moving to an abstraction where there is a "type" prop is likely too far an abstraction (also CancelButton, BackButton, CloseButton is sort of already the "types"). The original limitation I ran into with the Help section refactor was that I could not pass custom text for the one-off "Cancel" text needed.
We could create "types" including one for the "Cancel" scenario, but it sort of puts us right back into the original limitation I hit. What if we encounter a new context where we need "Dismiss" text? If we encounter a new context, do we want to add another "type" or pass the required text via a freeform text prop? WDYT?
Coming back to think I think once we have more specific accessibility Hint and specific content.
I think it would make sense to have a block for each of the different "BackButtons" s.
I don't have strong opinions about the granularity of the components in this case, but I wanted to mention that there may be cases where "Close" "Cancel" and "Back" are use somewhat inconsistently. The componentization in this PR might also be a good opportunity to clarify the intended semantics of these.
The last two quotes are from each of @enejb and @mkevins separately, but I think you are both saying the same thing — that you might prefer to keep CloseButton. If I am interpreting that correctly, that is fine with me. My original question of its existence is loosely held. I understand how creating it as a separate component to mange the text and any other accessibility properties provides value. 👍🏻
| } | ||
| /> | ||
| <BottomSheet.Header> | ||
| <BottomSheet.Header.CloseButton onPress={ close } /> |
There was a problem hiding this comment.
As questioned elsewhere, I think we may be better off passing the desired text here.
| <BottomSheet.Header.CloseButton onPress={ close } /> | |
| <BottomSheet.Header.CancelButton onPress={ close } text={ __( 'Close' ) } /> |
There was a problem hiding this comment.
I think you mean to write
<BottomSheet.Header.CancelButton onPress={ close } text={ __( 'Close' ) } />
Looking at the code I am not sure it is an improvement. Mostly since it creates confusion.
Maybe we should change it to?
<BottomSheet.Header.ReturnButton onPress={ close } text={ __( 'Close' ) } />
There was a problem hiding this comment.
I think you mean to write
<BottomSheet.Header.CancelButton onPress={ close } text={ __( 'Close' ) } />
Ah, yes. I did make a typo with the text string.
Looking at the code I am not sure it is an improvement. Mostly since it creates confusion.
Maybe we should change it to?<BottomSheet.Header.ReturnButton onPress={ close } text={ __( 'Close' ) } />
Confusion between the "CancelButton" component name and "Close" text? Possibly. If you believe renaming CancelButton provides more clarity, that works for me. ReturnButton or DismissButton could potentially work.
There was a problem hiding this comment.
Leaving one additional note that this conversation will be moot if we decide to retain the CloseButton, which I am open to doing.
| text-align: center; | ||
| font-weight: 600; | ||
| font-size: 16px; | ||
| position: absolute; |
There was a problem hiding this comment.
When I worked on the refactor for the Help section, I attempted to create the left-center-right layout while avoiding absolute positioning. I did this largely to reduce the likelihood of UI collisions. E.g. what happens with a really long title? Do you think there is any value in using the flex layout approach instead of absolute positioning?
There was a problem hiding this comment.
The problem that I ran into is that the title component wasn't centred anymore once I removed the empty View component that was making things centred.
I am not sure how we can make this happen without using absolute positioning. I will see if I can remove the absolute positing by using align-items: "center".
There was a problem hiding this comment.
Makes sense. If I am not mistaken, my CodePen I linked in my previous comment to does have the title centered even when the right button is not present. I used that in the original Help section header. So, I do believe it is possible.
There was a problem hiding this comment.
That said, it may not be worth the effort or complexity. Absolute position may work well for our use case.
| align-items: flex-start; | ||
| flex: 1; | ||
| justify-content: center; | ||
| z-index: 2; |
There was a problem hiding this comment.
If we avoid absolute positioning, it'd also likely negate the need for any z-index declarations.
| <BottomSheet.Header> | ||
| <BottomSheet.Header.BackButton onPress={ goBack } /> | ||
| <BottomSheet.Header.Title> | ||
| { label } | ||
| </BottomSheet.Header.Title> | ||
| </BottomSheet.Header> |
There was a problem hiding this comment.
I have largely seen "compound" components used to share state via React Context. I have not often seen "compound" components created largely for style application, i.e. providing the structural styles needed to keep the header UI consistent.
I think this use is really smart, and could work well for (1) ensuring the styles are applied consistently and (2) provide the ability to compose the elements as necessary for each context.
There was a problem hiding this comment.
BottomSheet.Header.Title
I am not really sure if this is desired. I kind of like it mostly because it gives you a context where the compone is meant to be used but this same "advantage" makes it harder to reuse the same component.
For example.
<BottomSheet.Footer>
<Header.Title>Some footer title</Header.Title>
</BottomSheet.Footer>
but I guess at that point we could refactor it to look be part of the bottom sheet title component. But all that could be avoided if we made the component a bottom sheet component at this point. Thought?
There was a problem hiding this comment.
Just a thought, but maybe "title" is not the best choice here 🤔 imho there should only be one title on a sheet.. so maybe label or heading is a better name?
There was a problem hiding this comment.
I agree with both of your perspectives. I suppose there is a fine line between naming the is too restrictive and naming so generic that it does not communicate intent.
I do like "heading" instead of "title." If we desire to allow these components to be used in a footer, we could replace "header" with "navbar."
<BottomSheet.NavBar>
<BottomSheet.NavBar.Heading>A Footer Component</BottomSheet.NavBar.Heading>
</BottomSheet.NavBar>
Leverage short, declarative statements when possible.
| ## BottomSheet.Header.CancelButton | ||
|
|
||
| Displays a styled button to dismiss a full screen bottom sheet. | ||
|
|
||
| ### Props | ||
|
|
||
| #### onPress | ||
|
|
||
| Callback invoked once the button is pressed. | ||
|
|
||
| ## BottomSheet.Header.CloseButton | ||
|
|
||
| Displays a styled button to dismiss a full screen bottom sheet. | ||
|
|
||
| ### Props | ||
|
|
||
| #### onPress | ||
|
|
||
| Callback invoked once the button is pressed. |
There was a problem hiding this comment.
Though the description here is the same for both of these components, I'm wondering about their intended semantics, and whether they really are the same. I think that Cancel implies "undoing" some changes that were made, while Close does not imply that. Otoh, what actually happens is entirely dependent on the implementation in the callback. Maybe we can find a way to define the difference between these components in the readme to guide developers that use these components toward a more consistent usage?
There was a problem hiding this comment.
Otoh, what actually happens is entirely dependent on the implementation in the callback.
That was my original thought with the documentation for the onPress callback. The current description is very much focused on what does occur, not the intended usage.
Maybe we can find a way to define the difference between these components in the readme to guide developers that use these components toward a more consistent usage?
Counter to the current documentation (I modified) and my aforementioned original thought, I do understand and agree there is value in describing the intent. The description of the components themselves differ somewhat from the onPress prop description. E.g. ApplyButton states "Displays a styled button to apply settings of bottom sheet controls," which describe the intent to apply the settings.
If we want to update the documentation to better describe the intent of the components and props, that is fine with me. Apologies if my previous changes removed too much of that, and thus creating more work to add the intent back. 😄
There was a problem hiding this comment.
Apologies if my previous changes removed too much of that, and thus creating more work to add the intent back.
No worries! I think it's great to discuss this and I'm happy this is being addressed. The usage of these patterns will certainly grow, so it's valuable to consider many paths. There is a tendency (and maybe a goal?) to make changes to block settings immediately reflected in the block rendering (i.e. no "confirm" or "cancel" needed). I think this can't cover the case for any fullscreen sheets, though, so this may be at least one source of tension. Historically, though, this may have been addressed in a somewhat ad-hoc way, giving rise to some redundancy in naming — but that's just my guess.
In any case, removing truly synonymous components and clarifying the usage of the remaining ones is a great idea. Also, I especially like the idea mentioned here to add iosText as a prop. This accurate signalling provides immediate clarity, whereas the current form feels like it is violating the principle of least surprise, imo.
packages/components/src/mobile/bottom-sheet/header/action-button.native.js
Outdated
Show resolved
Hide resolved
| function CloseButton( { onPress } ) { | ||
| return <CancelButton onPress={ onPress } text={ __( 'Close' ) } />; | ||
| } |
There was a problem hiding this comment.
I don't have strong opinions about the granularity of the components in this case, but I wanted to mention that there may be cases where "Close" "Cancel" and "Back" are use somewhat inconsistently. The componentization in this PR might also be a good opportunity to clarify the intended semantics of these.
| <BottomSheet.Header> | ||
| <BottomSheet.Header.BackButton onPress={ goBack } /> | ||
| <BottomSheet.Header.Title> | ||
| { label } | ||
| </BottomSheet.Header.Title> | ||
| </BottomSheet.Header> |
There was a problem hiding this comment.
Just a thought, but maybe "title" is not the best choice here 🤔 imho there should only be one title on a sheet.. so maybe label or heading is a better name?
|
Thanks for all the feedback and reviews @SiobhyB , @dcalhoun and @mkevins ! In my last update, I did the following:
I think things are in pretty good shape. I tested this as best as I could on iOS and Android and it seems it works now as before. Things to get done in different PRs. |
* trunk: (214 commits) Fix snackbar overflow on nav editor (#34661) Mobile - Allow disabling text and background color via theme.json (#34633) Fix disabled blocks logical error on Widgets screen (#34634) [Mobile] - Global styles - Add support to render font sizes and line height (#34144) ESLint Plugin: Update eslint jsdoc dependency (#34338) Scripts: Add CHANGELOG entry for `jest-dev-server` upgrade (#34657) Bump jest-dev-server to v5 (#34560) Refactor the `core-data` store to thunks (#28389) Only capture toolbars on parent Nav block when not in vertical mode (#34615) Update Changelog for 11.5.0-rc.1 Bump plugin version to 11.5.0-rc.1 Gallery block: Fix media placeholder height in site editor (#34629) Border Controls: Display color indicator and check selected color (#34467) Remove horizontal and vertical navigation block variations from inserter (#34614) AlignmentMatrixControl : Fix/update docs (#34624) Gap block support: force gap change to cause the block to re-render (fix Safari issue) (#34567) [Block Library - Social Links]: Use the new `flex` layout (#34493) [Mobile] Update the bottom sheet header (#34309) Group block: Add a row variation (#34535) Migrate entities.js to thunks (#34582) ...
Description
This PR Refactors the Navigation Header Component so that it is more compostable, easier to understand and updates the usage of the component.
How has this been tested?
Bottoms sheets to test out.
Screenshots
The Bottom Sheet Header should look exactly the same as before.
Something like this.
Types of changes
Refactor
Checklist:
*.native.jsfiles for terms that need renaming or removal).