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

Doc Blocks: Add support for of prop to Primary block #23849

Merged
merged 26 commits into from
Oct 19, 2023

Conversation

Wilson2k
Copy link
Contributor

Partially implements #22490

What I did

How to test

  1. Run yarn storybook:blocks
  2. Open Storybook in your browser
  3. Access Primary stories

Checklist

  • [x ] Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • [x ] Make sure to add/update documentation regarding your changes
  • [x ] If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]

@Wilson2k
Copy link
Contributor Author

Hi everyone, this is my first contribution to the codebase. Please let me know if I can improve anything, thanks!


const meta = {
const meta: Meta<typeof Primary> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why you changed from the satisfies operator to direct assignment of the type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, thanks for reviewing my code! I changed it based off other PRs and off of Description.stories.tsx. But this is a good question, and after some research, I'll change it back to satisfies since it's just better for validation and type safety.


Type: CSF file exports

Specifies the primary (first) story to be rendered.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we state something similar to Meta's of prop description?

Suggested change
Specifies the primary (first) story to be rendered.
Specifies which CSF file is used to find the first story, which is then rendered by this block. Pass the full set of exports from the CSF file (not the default export!).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure thing, I'll push a commit with the other fix today.

Copy link
Contributor

@jonniebigodes jonniebigodes left a comment

Choose a reason for hiding this comment

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

@Wilson2k, thanks for addressing the feedback so promptly. Appreciate it 🙏 ! Documentation-wise, it's all good on my end. I'll defer to the other maintainers to see if they have anything else to say about this great contribution.

cc @JReinhold
Hope you have a great day.

Stay safe

@yannbf
Copy link
Member

yannbf commented Oct 2, 2023

Hey @JReinhold could you please take a look?

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.

Thanks for the PR and the patience, generally looks good! A few minor changes needed, but then it's ready!

code/ui/blocks/src/blocks/Primary.tsx Outdated Show resolved Hide resolved
MIGRATION.md Outdated
Comment on lines 1507 to 1510
##### Primary block

The `Primary` block now also accepts an `of` prop as described above. It still accepts being passed `name` or no props at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

These migration guides are between specific versions, adding this note here communicates that this was introduced in 7.0, while it's probably being introduced in 7.6.

I suggest you create a new top section below the Table Of Content called "From version 7.5.0 to 7.6.0", that includes this content. Given that this will move it "out" of the doc block section, you probably need to add a bit more context and link to this doc block section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok sounds good, I'll push a commit along with the other one updating the docs.

@Wilson2k Wilson2k requested a review from JReinhold October 16, 2023 18:34
Comment on lines 52 to 53
const resolvedOf = useOf(of || 'meta', ['meta']);
story = getPrimaryFromResolvedOf(resolvedOf);
Copy link
Contributor

@JReinhold JReinhold Oct 17, 2023

Choose a reason for hiding this comment

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

I'm sorry for not realising this sooner, I just thought of it now. I think now that we supply 'meta' as the only valid type, we don't need the "complex" error handling in getPrimaryFromResolvedOf, as that is already done within useOf based on the second argument - it should even be typed correctly.

So maybe you could try this and remove getPrimaryFromResolvedOf entirely and see if that still works?

Suggested change
const resolvedOf = useOf(of || 'meta', ['meta']);
story = getPrimaryFromResolvedOf(resolvedOf);
const resolvedOf = useOf(of || 'meta', ['meta']);
story = resolvedOf.csfFile.stories[0] || null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, sorry for the late reply, I just tested it and it seems to work fine. Good catch, I'll push a commit with the changes.

@JReinhold JReinhold changed the title Updated Primary block to support of prop Doc Blocks: Add support for of prop to Primary block Oct 19, 2023
@JReinhold JReinhold merged commit de82e37 into storybookjs:next Oct 19, 2023
10 checks passed
@github-actions github-actions bot mentioned this pull request Oct 19, 2023
11 tasks
@Wilson2k Wilson2k deleted the update-primary-block-of-prop branch October 19, 2023 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants