-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fix(storybook): support mdx #5456
fix(storybook): support mdx #5456
Conversation
✅ Deploy Preview for redwoodjs-docs canceled.
|
Hey Peter ! Is it ready for review ? I'm excited to try out 😄 |
👋🏻 hi @simoncrypta , yes it is ready. I was going to add an MDX file to our smoke test to verify; but please feel free to try it out if you are excited. |
6563f09
to
76706af
Compare
@simoncrypta in order to get this to work it looks like we need to @shilman - any hint as to why even with We are wiring it up here but we also do some stuff in there that may be affecting how Storybook expected to be configured. (cc @jtoar) |
storybookjs/storybook#14429 looks related |
78064d2
to
4b7b67d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, but a couple of minor comments!
4b7b67d
to
9eb9334
Compare
9eb9334
to
32c358d
Compare
32c358d
to
1a94637
Compare
👍I think the duplication is okay really - unless you wanted to move the mdx into a string - but not sure how much cleaner that is! I know it's weird about the codemod folder - but short of renaming applyCodemod to applyCodeshift - not sure what else we could do that's an improvement. Atleast this way we know all the code in this folder will change the project somehow. I just fixed the rebuild fixture with Dom’s help a few PRs ago btw - so you can actually regenerate the fixture now with |
1a94637
to
dd4f8fc
Compare
@dac09 - I know this is your favorite topic so please don't 🔫 the messenger, but it seems like we have some flakey CI/CD tests. These failed due to timeout, when every other run passed -
It seems mostly unrelated to this PR, but if adding MDX to Storybook increases the load on CI/CD we might want to rethink adding it as a smoke test. |
Timeouts usually mean storybook hasn't started up correctly I'm afraid. The timeout happens because the tests are looking for the elements, but can't find it. Also just noticed there aren't actually any smoke tests for mdx haha. Up to you if you want to add it - I really don't want to be master of smoke tests. |
2292b35
to
1459916
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks LGTM! Awaiting confirmation on where @storybook/addon-docs
goes.
I personally don't think we need to modify the template, because rwjs/testing (which now includes addon-docs) is already is a dependency - but will defer to someone else on this.
e463647
to
05d42fe
Compare
05d42fe
to
cba83d4
Compare
cba83d4
to
19e8b7f
Compare
closes redwoodjs#5348 build(storybook): take Storybook's MDX rule Co-authored-by: Dominic Saadi <[email protected]>
ae2b6d3
to
3303cfe
Compare
closes #5348