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

Convert JS stories to MDX #584

Closed
1 task
dhruvkb opened this issue Apr 1, 2022 · 11 comments · Fixed by #1089
Closed
1 task

Convert JS stories to MDX #584

dhruvkb opened this issue Apr 1, 2022 · 11 comments · Fixed by #1089
Assignees
Labels
💻 aspect: code Concerns the software code in the repository ✨ goal: improvement Improvement to an existing user-facing feature 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: frontend Related to the Nuxt frontend 🧩 tech: storybook Requires familiarity with Storybook

Comments

@dhruvkb
Copy link
Member

dhruvkb commented Apr 1, 2022

Problem

JS stories have many shortcomings compared to MDX. While MDX stories can be cumbersome, they really shine when it comes to being able to write long-form documentation, including helpful tips about the props, functionality and expected use-cases of components. In a sense, they're almost equivalent to having a README.md for every component.

This has been spun-off from this discussion.

Description

The goal is to convert the existing JS stories to their MDX counterparts. To keep PRs reviewable, please do only one or two components at a time.

Alternatives

We can continue to use a mix of JS and MDX stories as currently done. This is bad because why would you mix two ways of doing one thing in one project.

The other alternative is to go the other route (aka backwards) and convert all MDX stories to the less flexible JS format. This would add consistency but at a huge price.

Additional context

There are a lot of MDX stories in the repo to refer to. Just pick the longest, more comprehensive one as a reference.

Implementation

  • 🙋 I would be interested in implementing this feature.
@dhruvkb dhruvkb added good first issue New-contributor friendly help wanted Open to participation from the community 🟩 priority: low Low priority and doesn't need to be rushed ✨ goal: improvement Improvement to an existing user-facing feature 💻 aspect: code Concerns the software code in the repository labels Apr 1, 2022
@obulat
Copy link
Contributor

obulat commented May 5, 2022

Stories that still need to be converted:

@dhruvkb
Copy link
Member Author

dhruvkb commented May 5, 2022

Thanks for adding the checklist @obulat. Very helpful to keep a track!

@sarayourfriend
Copy link
Collaborator

Just wanted to point out here what had been mentioned in the WP Make Slack (needs an account, which you can create here).

Basically MDX does not currently have TypeScript support. If it ever did, because of it's nature as a React based extension on Markdown, it will necessarily require the @types/react package which is wholly incompatible with the Vue @vue/runtime-dom types that are used for template checking (which is a "pseudo-JSX" in the view of the TypeScript compiler). Both packages write to the global.JSX namespace that TypeScript relies on to type check JSX.

The JS stories, on the other hand, could be converted to TypeScript and included into the TypeScript project (potentially with similar issues around getting type resolution, but at least then we can try other dependency resolution or TypeScript tricks to remove the @types/react package because we won't be writing any React JSX that needs to be type checked, which is unavoidable with the MDX approach, there you must write React JSX.

If we go the MDX route, then we probably have to accept that the stories will not be able to be type checked any time in the foreseeable future, not until some compatibility shim is created to make React JSX types and Vue JSX types live happily together (seems unlikely to me given the communities have basically no overlap outside of Storybook).

@obulat
Copy link
Contributor

obulat commented May 5, 2022

It seems that we have to choose between having the ability to document the components in prose using MDX and the ability to type-check components in Storybook.

I really like how we can write about the components in the Docs tab. I think we should use this even more than we are using now. We could add more details about when to use which component or which variant of the component. The example that comes to mind is the buttons, which have some variants that currently seem to overlap.

Can we say that we can rely on the app code and the unit tests for the correct typing in the component, and leave the Storybook components without the check? The types are usually shown correctly in the args table. If you rely on the Storybook for examples of how to use a component in the app, it will check your types there and warn you if necessary.

@sarayourfriend
Copy link
Collaborator

We can still write stories as prose, of course, just not as Markdown. Or we even could use Markdown if we wanted, using, for example, something like this component (but maybe not it exactly, I haven't evaluated it closely as an acceptable dependency or not).

Stories could be written fully as a Vue component in the meta folder, with the stories.js just configuring the Storybook meta and rendering each prose-style story.

// components/VMagic/VMagic.vue
<template>
  <!-- etc -->
</template>
// components/VMagic/meta/VMagic.DefaultStory.vue
<template>
  <div>
    <VMarkdown>
      ## VMagic

      `VMagic` is a component with magical super powers.

      <!-- etc ... -->
    </VMarkdown>
    <VMagic v-bind="$attrs">
      <!-- etc ... ->
    </VMagic> 
  </div>
</template>
// components/VMagic/meta/VMagic.stories.ts
import VMagicDefaultStory from '~/components/VMagic/meta/VMagic.DefaultStory.vue'

export default {
  name: 'Components/VMagic'
}

export const Default = (args) => ({
  template: '<VMagicDefaultStory v-bind="args" />',
  components: { VMagicDefaultStory },
  setup() { return { args } },
})
// etc...

It's not as straightforward as MDX (if you consider MDX to be straightforward) but considering the stories are the place where we're meant to exercise the components to the fullest, it does seem wise to me for them to also by type checked.

@obulat obulat transferred this issue from WordPress/openverse-frontend Feb 22, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Openverse Backlog Feb 23, 2023
@obulat obulat added the 🧱 stack: frontend Related to the Nuxt frontend label Feb 24, 2023
@sepehrrezaei
Copy link
Contributor

Hi, I would like to contribute on this matter. Could you please assign it to me?

@sarayourfriend sarayourfriend added 🧹 status: ticket work required Needs more details before it can be worked on and removed good first issue New-contributor friendly help wanted Open to participation from the community labels Mar 26, 2023
@sarayourfriend
Copy link
Collaborator

@sepehrrezaei Apologies, but I think this issue might need some additional ticket work to decide how the project best wants to solve it.

@dhruvkb and @obulat do y'all have any objection to the approach I shared above? I do think it's important that the stories are type checked.

@dhruvkb
Copy link
Member Author

dhruvkb commented Mar 27, 2023

@sarayourfriend I still prefer MDX because of the following reasons.

  • The Docs page can actually be long-form docs with some supporting stories mixed into it, which is the opposite of your suggestion where each story has some documentation mixed into it. This approach will leave the Docs page empty.
  • MDX keeps the documentation outside the story, so the code for the story is concerned purely with the component while this approach will leak documentation into the individual stories tab.
  • I find the built-in doc-page components such as ArgsTable, Description etc. seem very useful and worth keeping around. They cannot be used if the documentation is written inside .vue components.
  • Typechecking of stories, while important, seems like something I can compromise on and my mind would be at ease because stories have (or should have) VR tests and the rest of the app has other tests (unit, e2e and VR).
  • I actually consider MDX to be amazingly straightforward, except for some boilerplate in the start to set things up. The long-form docs aspect of MDX is purely Markdown, that anyone can read and write fairly easily.

To summarise, personally, I favour MDX and am willing to lose type safety in the stories to use it. However, if you could make a PR, we can all see how the end result looks (from the PR preview) and make a final call on what works.

@sarayourfriend
Copy link
Collaborator

The Docs page can actually be long-form docs with some supporting stories mixed into it, which is the opposite of your suggestion where each story has some documentation mixed into it. This approach will leave the Docs page empty.

Ahhh, I did not realise exactly what this meant before, but now I do. I agree then that my suggestion is not an adequate solution to the issue.

@sepehrrezaei
Copy link
Contributor

@sarayourfriend & @dhruvkb So after your discussions, Can I work on it ?

@sarayourfriend sarayourfriend removed the 🧹 status: ticket work required Needs more details before it can be worked on label Mar 27, 2023
@sarayourfriend
Copy link
Collaborator

@sepehrrezaei Yup! I've assigned the issue to you 👍 Thanks for hopping in.

@krysal krysal added the 🧩 tech: storybook Requires familiarity with Storybook label Mar 30, 2023
@obulat obulat moved this from 📋 Backlog to 🏗 In progress in Openverse Backlog Apr 3, 2023
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Openverse Backlog Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 aspect: code Concerns the software code in the repository ✨ goal: improvement Improvement to an existing user-facing feature 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: frontend Related to the Nuxt frontend 🧩 tech: storybook Requires familiarity with Storybook
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants