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

Docs: Refactor props of Story block #20530

Merged
merged 17 commits into from
Jan 17, 2023
Merged

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Jan 7, 2023

Breaking prelease change: This changes the docs.autoplay parameter to docs.story.autoplay

Issue: N/A

What I did

  • Big refactor of the Story block/component that was long overdue.
  • Rename docs.inlineStories parameter to docs.story.inline.
  • Rename iframeHeight to story.height.
  • Rename autoplay to story.autoplay.
  • Add autoplay prop to

How to test

  • E2E tests + blocks stories.
  • We need to check external docs manually.

@tmeasday tmeasday added maintenance User-facing maintenance tasks addon: docs labels Jan 7, 2023
@tmeasday tmeasday requested review from shilman and JReinhold January 7, 2023 05:16
@tmeasday
Copy link
Member Author

tmeasday commented Jan 9, 2023

@JReinhold as I looked at the stories for these blocks (we should look at these stories together), I discovered this PR drops one piece of functionality from the Preview block, that maybe we hadn't realised -- using the layout parameter (note no docs namespace).

How it works currently (on next):

  1. The Story block reads the story's parameters
  2. It passes them to the Story component as parameters prop
  3. The Story component ignores that prop (which is why I dropped it)
  4. However the Preview component reads the props off its children, and ends up finding the parameters and reading the layout.

This is pretty obtuse, probably it would be better if instead:

  1. The Canvas block figures out the stories rendered by its children
  2. It then calculates a single layout prop for the Preview component.

Let's have a chat about this.

@JReinhold
Copy link
Contributor

@JReinhold as I looked at the stories for these blocks (we should look at these stories together), I discovered this PR drops one piece of functionality from the Preview block, that maybe we hadn't realised -- using the layout parameter (note no docs namespace).

How it works currently (on next):

  1. The Story block reads the story's parameters
  2. It passes them to the Story component as parameters prop
  3. The Story component ignores that prop (which is why I dropped it)
  4. However the Preview component reads the props off its children, and ends up finding the parameters and reading the layout.

This is pretty obtuse, probably it would be better if instead:

  1. The Canvas block figures out the stories rendered by its children
  2. It then calculates a single layout prop for the Preview component.

Let's have a chat about this.

I see, that's not an ideal API.
We recently decided that Canvas will only support a single story via the of prop (and thus no children), so I think the way to handle this is with a layout prop on Canvas and a matching parameters.docs.canvas.layout option. But keeping the parameters.layout option for use in story mode. It's strange to have both parameters, let's chat about that, we have the same situation with ArgTypes and Controls.
https://www.notion.so/Autodocs-678fd5d85cab4f2497219cf110263bcc?d=d8ee23a4df7d42e3ae49bec5810c3d28#9b9da7d2107e4296bcc1dd10782403d7

code/ui/blocks/src/examples/Button.stories.tsx Outdated Show resolved Hide resolved
code/ui/blocks/src/blocks/Story.stories.tsx Outdated Show resolved Hide resolved
code/ui/blocks/src/blocks/Story.tsx Outdated Show resolved Hide resolved
@tmeasday tmeasday requested a review from JReinhold January 10, 2023 05:57
@tmeasday
Copy link
Member Author

Ok, I think this is good now @JReinhold. There was some confusion around height vs iframeHeight. I've clarified in the notion docs, hopefully it's clear to you:

image

image

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

Looks good!
What's stopping us from using useOf in the Story block for consistency? I could imagine that the nature of the Story block and still supporting the deprecations would make it unwieldy to use. Or is it because you built this before the useOf was merged? Either way it's okay to me.

code/ui/blocks/src/blocks/Story.stories.tsx Show resolved Hide resolved
@@ -51,7 +51,7 @@ export const SimpleSizeTest: Story = {
style={{
Copy link
Contributor

Choose a reason for hiding this comment

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

When changing the stories from Boolean Control to Button I missed the one that uses id above on line 33. could you change it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Although should this be in an internal story file do you think?

@tmeasday
Copy link
Member Author

What's stopping us from using useOf in the Story block for consistency?

It was just that code was already written in the PR that added resolveModuleExport. Honestly though I am not sure how I feel about useOf() due to the comments mentioned in #20603 (comment). We can talk about it?

@tmeasday
Copy link
Member Author

@JReinhold -- what shall we do with the unaccepted changes here? They are stories that are currently broken due to the discussion here: #20530 (comment) -- shall we accept them as is or disable them for now?

@JReinhold
Copy link
Contributor

@JReinhold -- what shall we do with the unaccepted changes here? They are stories that are currently broken due to the discussion here: #20530 (comment) -- shall we accept them as is or disable them for now?

Let's accept them, otherwise they'll spring up in other unrelated PRs and confuse the team.

@tmeasday
Copy link
Member Author

Ok, they are accepted but hopefully you can fix them reasonably quick, I'm not super comfortable with having stories that are just wrong.

@tmeasday
Copy link
Member Author

I'll update the useOf in the same PR mentioned here: #20603 (comment)

@tmeasday tmeasday merged commit fa16ccb into next Jan 17, 2023
@tmeasday tmeasday deleted the tom/sb-1144-update-api-of-story-block branch January 17, 2023 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: docs maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants