Update Navigator styling to facilitate sticky positioning
#35518
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stokesman
added
[Type] Enhancement
7 tasks
|
Size Change: +184 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
7 tasks
ciampo
reviewed
Oct 11, 2021
There was a problem hiding this comment.
Thank you for working on this change, it's very appreciated (especially given the effort you also put into adding a detailed description and a Storybook example!)
I think this is a good change to introduce. The component is also still experimental, and therefore I think we have some room to make this kind of changes without worrying too much. On top of this, it is a quite minor change which shouldn't affect current usages of the component.
The only comments I made are about the Storybook example. You may mark this PR ready for review once you're done answering the feedback, and it will probably be ready to be merged!
| <p>This is the child screen.</p> | ||
| <NavigatorButton isPrimary path="/" isBack> | ||
| Go back | ||
| <NavigatorButton path="/overflow-child"> |
There was a problem hiding this comment.
Unrelated to the changes in this PR, but — should mark this button (and all NavigatorButton) as variant="primary" ?
There was a problem hiding this comment.
Ah, I was making some tangential changes and it's good to review them. So besides updating to the variant prop, I was trying to follow the button placement guidelines, specifically:
Don’t: Don’t place two primary buttons next to one another — they compete for focus. Only use one primary button per view.
So for a screen that only has a back back button it's probably fine to make primary, though I felt it was probably okay as default since an actual screen probably wouldn't consider the back button primary.
I'm open to suggestions. An idea I've considered is having the root screen use a ItemGroup instead of individual buttons.
There was a problem hiding this comment.
That's a good point. I guess I only wanted to differentiate the "links" from the button opening the modal — maybe we could then use secondary instead?
Something like this
diff --git a/packages/components/src/navigator/stories/index.js b/packages/components/src/navigator/stories/index.js
index fe40eb67a1..97e527216e 100644
--- a/packages/components/src/navigator/stories/index.js
+++ b/packages/components/src/navigator/stories/index.js
@@ -22,6 +22,7 @@ function NavigatorButton( { path, isBack = false, ...props } ) {
const navigator = useNavigator();
return (
<Button
+ variant="secondary"
onClick={ () => navigator.push( path, { isBack } ) }
{ ...props }
/>
@@ -41,7 +42,7 @@ const MyNavigation = () => {
<p>This is the home screen.</p>
<HStack justify="flex-start" wrap>
- <NavigatorButton variant="primary" path="/child">
+ <NavigatorButton path="/child">
Navigate to child screen.
</NavigatorButton>
@@ -54,7 +55,11 @@ const MyNavigation = () => {
</NavigatorButton>
<Flyout
- trigger={ <Button>Open test dialog</Button> }
+ trigger={
+ <Button variant="primary">
+ Open test dialog
+ </Button>
+ }
placement="bottom-start"
>
<CardHeader>Go</CardHeader>There was a problem hiding this comment.
Thanks! I like it and have applied it.
ciampo
approved these changes
Oct 12, 2021
There was a problem hiding this comment.
There's only one minor comment left. Once that's solved, we should go ahead and merge this PR! Thank you again, @stokesman !
Co-authored-by: Marco Ciampini <[email protected]>
This was referenced Oct 13, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is mostly to expound on a point from #35369 and contains a tiny change with a long story (Storybook). Since #35332, if an implementor of
Navigatorhas content styled withposition: stickyit will have to add some styling toNavigatorto have it and its first childNavigatorScreennot exceed the height of their parent scrollable context. Without doing so, the component itself may overflow and the sticky elements won't stick where intended. For a visual demonstration see the Storybook screen capture here or #35369 for an example within Gutenberg.This PR proposes a style to ease the implementors task a bit. We can add
max-height: 100%toNavigatorScreenand it's not really opinionated. It does nothing when the component root doesn't have a specified height and facilitates the styling task for an implementor since they only need to specify the height of the root element.Alternatives Considered
Add a prop to
Navigatorthat adds the styling to match the height of its parent. Example:This could work but depending on the style implementation may not suit every use case. To me, it feels like too much to think about for what seems an uncommon use case.
Another alternative is to simply add a note in the component’s readme to advise about styling for sticky situations.
How has this been tested?
Manually in Storybook
Screenshots
Storybook demonstration
navigator-screen-max-height.mp4
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).