Skip to content

refactor: Migrate modal story to pure MDX story format#520

Merged
nats12 merged 8 commits intodevfrom
natalie/mdx_modal_template
May 18, 2021
Merged

refactor: Migrate modal story to pure MDX story format#520
nats12 merged 8 commits intodevfrom
natalie/mdx_modal_template

Conversation

@nats12
Copy link
Copy Markdown
Contributor

@nats12 nats12 commented May 11, 2021

Short description

Converted the modal story into pure MDX format as we start to move towards MDX documented components.

Ref: https://twist.com/a/1585/ch/190200/t/2455247/

This involved extracting all functions from the helper file and placing them into the MDX file alongside the stories. I've tried to reuse as much code as possible here, defining templates where possible, but please feel free to suggest any other ways in which I could make these more reusable.

PR Checklist

  • Make sure the code makes sense
  • Make sure that all stories under Modal look and work the same way as before when clicking on their trigger buttons see deployed version for reference
  • Make sure all the controls under the Modal Playground Story work as before

Versioning

v9.1.5 will be released to include this story

@nats12 nats12 requested a review from frankieyan May 11, 2021 13:28
Copy link
Copy Markdown
Member

@frankieyan frankieyan left a comment

Choose a reason for hiding this comment

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

This is looking really good 👍

Random thoughts, but they don't need to be a part of this PR:

  • I wonder if there is value in having a separate template .mdx file, because the modal here is a bit of a different use case where it's made up of several sub-components, so our ArgsTable is currently a bit different. It'd be good to have an example where our props are inferred from the components themselves
  • Also if we have a separate template, should we define some placeholders for information we should fill out? ie. component description, use cases, anything else we can think of? For reference, this is the fullstack team's storybook: https://doist.github.io/marketist/?path=/docs/typography-text--standard
  • How do we add the description and default fields to the args table? Is it through jsdocs, or a defaultProps property on the component?

Comment thread stories/components/Modal.stories.mdx Outdated
Comment thread stories/components/Modal.stories.mdx
Comment thread stories/components/Modal.stories.mdx Outdated
so <ArgsTable of={Modal} /> wouldn't work in this case. Instead, we specify the
props of one story i.e. Playground
-->
<ArgsTable story="ModalPlaygroundStory" />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be possible to show the individual components' props? Storybook's docs imply that this is possible, but I tried updating meta to:

<Meta
    title="Modal"
    component={Modal.Box}
    subcomponents={{ Header: Modal.Header }}
/>

and this to <ArgsTable of={Modal.Box} />, but the prop types aren't inferred:

image


Also, it's a bit funny that the props here are toggleable but the story itself sits at the very bottom. If the above isn't possible, then perhaps we can list the playground story first?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've tried doing this as well thinking each of Modal's subcomponent types would get picked up but they don't. I've even tried importing Box directly and setting that as a subcomponent as well as ArgsTable's of value

@nats12
Copy link
Copy Markdown
Contributor Author

nats12 commented May 13, 2021

TL;DR: a) I think you're right and Modal might not be the best story to use as reference and b) getting prop types inferred for this component is proving to be tricky even without the MDX implementation because its made up of multiple nested components. This may be the case with a few other components using this same approach i.e. Dropdown

I wonder if there is value in having a separate template .mdx file, because the modal here is a bit of a different use case where it's made up of several sub-components, so our ArgsTable is currently a bit different. It'd be good to have an example where our props are inferred from the components themselves

So I've been trying out different implementations to get prop types inferred with Modal but can't get it to work. Removing the MDX file and going for the standard DocsPage implementation doesn't seem to generate the table either (I've tried applying the defaultProps property as well). So I think you may be right in that this is a special case which may not be the best to use as a reference, but we also need to keep in mind that we may not be able to show a props table for this with either approach.

Also if we have a separate template, should we define some placeholders for information we should fill out? ie. component description, use cases, anything else we can think of? For reference, this is the fullstack team's storybook: doist.github.io/marketist/?path=/docs/typography-text--standard

If this is regarding Modal this would be a good idea although it might overcrowd the template. I've added different sections for helpers, template binds etc - did you mean extra comments for things like where descriptions could go within each template bind?

How do we add the description and default fields to the args table? Is it through jsdocs, or a defaultProps property on the component?

The descriptions get added to the table automatically when the props are inferred from the components, which sadly isn't the case in Modal's case. We can define them manually, however the docs specify that this customisation is only available using the <ArgsTable story=<name of story> /> format instead of the <ArgsTable of={<component>}> /> format we are after. So the prop would have to go on each story looking similar to what you tried before:

<ArgsTable story={"ModalHeaderOnlyStory"} />
.... 

<Story
    ....
    argTypes={{
        title: {
             description: 'some description'
         }
     }}
....
</Story>

The problem with this is that we would have to have a table for each story and the same props often show up on each story.

@frankieyan
Copy link
Copy Markdown
Member

In that case the changes here for the modal looks good to me 👍 we can investigate issues with the props table separately - it would be awesome if descriptions and default values can be inferred, or if at least there is an easy way to define them.

For now, what do you think about bringing the playground story closer to the props table (as the first story)?

Another thing is that it looks like the stories themselves can have a name defined, rather than using the exports' names: https://storybook.js.org/docs/react/writing-stories/introduction#rename-stories. Should we do this here (with just the modal to start?)

@nats12
Copy link
Copy Markdown
Contributor Author

nats12 commented May 14, 2021

Another thing is that it looks like the stories themselves can have a name defined, rather than using the exports' names: storybook.js.org/docs/react/writing-stories/introduction#rename-stories. Should we do this here (with just the modal to start?)

Yes this is a good idea, we are already setting the story's name property anyway so why not give them better names.

For now, what do you think about bringing the playground story closer to the props table (as the first story)?

@frankieyan This would make sense as it could get confusing toggling the controls and not seeing the changes being reflected in any of the docs tab stories. Something to keep in mind with this though is that it would mean Playground would appear first under Modal within the sidebar

image

Comment thread stories/components/Modal.stories.mdx Outdated
Comment thread stories/components/Modal.stories.mdx
Comment thread stories/components/Modal.stories.mdx
@nats12 nats12 merged commit a05f186 into dev May 18, 2021
@nats12 nats12 deleted the natalie/mdx_modal_template branch May 18, 2021 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants