Skip to content
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

Merged
merged 13 commits into from
Aug 28, 2024

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Aug 26, 2024

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?

  • better spacing, no "cutoff" overflowing styles
  • removed Card components (they are not required to show how Navigator works)
  • better clarity, typography, and semantics of screen titles
  • cleaned up some outdated code

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

  • Load Storybook
  • Make sure that the examples continue to work as expected

@ciampo ciampo requested a review from ajitbohra as a code owner August 26, 2024 10:51
@ciampo ciampo requested a review from a team August 26, 2024 10:51
@ciampo ciampo self-assigned this Aug 26, 2024
Copy link

github-actions bot commented Aug 26, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Member

@mirka mirka left a 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 🚀

packages/components/src/navigator/stories/index.story.tsx Outdated Show resolved Hide resolved
packages/components/src/navigator/stories/index.story.tsx Outdated Show resolved Hide resolved
</CardBody>
</Card>
</NavigatorScreen>
<StyledNavigatorScreen path="/">
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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 🙈

Copy link
Contributor Author

@ciampo ciampo Aug 27, 2024

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.

Copy link
Member

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="/">
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

@ciampo ciampo Aug 27, 2024

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huge improvement, thank you 🙇

@ciampo ciampo force-pushed the feat/stabilize-navigator/storybook-improvements branch from 3e419bb to 1c27fbe Compare August 27, 2024 16:50
@ciampo
Copy link
Contributor Author

ciampo commented Aug 27, 2024

@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

@ciampo ciampo requested a review from mirka August 27, 2024 17:01
Copy link
Member

@tyxla tyxla left a 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 🚀

@ciampo ciampo merged commit fdff00c into trunk Aug 28, 2024
67 of 69 checks passed
@ciampo ciampo deleted the feat/stabilize-navigator/storybook-improvements branch August 28, 2024 17:00
@ciampo
Copy link
Contributor Author

ciampo commented Aug 28, 2024

Thank you for the review — always happy to make more incremental changes in follow-ups.

@github-actions github-actions bot added this to the Gutenberg 19.2 milestone Aug 28, 2024
bph pushed a commit to bph/gutenberg that referenced this pull request Aug 31, 2024
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants