-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Navigator: polish Storybook examples #64798
Navigator: polish Storybook examples #64798
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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.
To be honest I still have difficulty with the overall state of our stories here, but this is a good cleanup nonetheless 🚀
</CardBody> | ||
</Card> | ||
</NavigatorScreen> | ||
<StyledNavigatorScreen path="/"> |
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.
Is it necessary for this Nested Navigator story to take up this much vertical space?
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.
I thought it would be a good practice to set each screen to be full height — do you think it's actually a bad practice?
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.
Honest question — why is it a good practice? Does it ensure that the screens animate properly or something? I'm not familiar enough with the component to know why it should be that tall. As a consumer I just see some relatively short content and a lot of whitespace 🙈
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.
Good questions! I looked a bit more into why historically (and instinctively) these stories had height
styles.
It's not important for the screens to be full height or to have height: 100%
.
What matters is that the NavigatorProvider
(soon to be renamed Navigator
) wrapper has enough height to host the tallest screen. Otherwise, when transitioning between screens (and especially with the changes proposed in #64777), the content may "jump" (or get cut off) unexpectedly.
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.
Thanks for investigating, that makes sense! Maybe something to document as well.
<Card> | ||
<CardBody> | ||
<p>This is the home screen.</p> | ||
<StyledNavigatorScreen path="/"> |
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.
Out of scope for this PR, but as a consumer I have difficulty understanding this story. It seems like a grab bag demo of features, including some "tests" for edge cases which aren't particularly useful to a consumer.
Since we're stabilizing this component, I think we can improve the stories a bit more to highlight features and common use cases in a consumer-centric way. Story descriptions would also help.
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.
I'll see if I can tidy up the examples in this PR — stay tuned
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.
I've updated the stories:
- removed "edge case" screens (overflowing content, sticky headers)
- removed "Nested navigator" example, since it's now the default behavior anyway
- added a "with initial nested path" story
- removed external components (dropdown) from the story
- renamed "product screen" to "dynamic screen", and added more details about it in the screen's body text.
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.
Huge improvement, thank you 🙇
3e419bb
to
1c27fbe
Compare
@mirka let me know if this is ready to be merged after the recent changes, or if there's anything else that we should address |
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.
Great cleanup, thanks @ciampo!
I've done some testing and all stories worked well 🚀
Thank you for the review — always happy to make more incremental changes in follow-ups. |
* Remove subfolder index.ts files, since they interfere with Storybook docgen * Use MenuItem for Dropdown, remove unnecessary types * Remove Card usage from the Storybook example * Fix sticky screen styles after removal of Card * Set inline padding and height on every screen * Better title semantics and typography * Add one more link to product details screen to show how it works * Refactor skip focus styles * Avoid double y-scrolling on full-height container * Move back wrongly positioned metaphor ipsum * Move navigator styles to decorator * Tweak screen and provider styles * Simplify Storybook examples, removing edge cases and focusing on standard usage --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
What?
Part of #59418
Extracted from #64777
While working on a few
Navigator
-related PRs, I accumulated a few small tweaks and improvements to the Storybook files.Why?
Mostly small tweaks that don't dramatically change how the Storybook examples work and look, but iteratively improve the code quality and the examples' usefulness and sense of polish.
How?
Card
components (they are not required to show howNavigator
works)I also removed the intermediary
index.ts
files, since I found out that the lack of.tsx
extension can interfere with Storybook's docgen.Testing Instructions