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(Modal): add storybook MDX example #6804

Merged
merged 12 commits into from
Sep 28, 2020

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Sep 9, 2020

Ref #6591

This PR adds a custom modal state manager example and MDX docs to the existing Modal docs

Testing / Reviewing

Review the new modal state manager example and the storybook Docs tabs updates

@netlify
Copy link

netlify bot commented Sep 9, 2020

Deploy preview for carbon-elements ready!

Built with commit 5791197

https://deploy-preview-6804--carbon-elements.netlify.app

@emyarod emyarod mentioned this pull request Sep 9, 2020
36 tasks
@netlify
Copy link

netlify bot commented Sep 9, 2020

Deploy preview for carbon-components-react ready!

Built with commit 5791197

https://deploy-preview-6804--carbon-components-react.netlify.app

@joshblack
Copy link
Contributor

@emyarod looking through the Modal PRs, would it make sense if we consolidate them into a single docs page and maybe grouped them under a "Modal" group? Just saw some of the content here included ComposedModal which made me think of it 👀

@emyarod
Copy link
Member Author

emyarod commented Sep 10, 2020

yeah that's a side effect of moving from the website to storybook where we have distinct exported components. so would that mean grouping the stories together or something?

@tw15egan
Copy link
Collaborator

tw15egan commented Sep 10, 2020

Could you do something like (in ComposedModal-story.js) import mdx from '../Modal/Modal.mdx' ? Basically just reference the same MDX files in each of them.

Haven't tested so not sure it's possible.

@emyarod
Copy link
Member Author

emyarod commented Sep 11, 2020

yeah we can do that if we are fine with combining the docs for the modal variants, but the component previews will all be the same as well

@joshblack
Copy link
Contributor

I think we could definitely do a Modal group and group each sub-component under it while providing a holistic "how to use modal" docs page. The other option could be that we each docs page refer to only the component in the story (e.g. Modal.mdx would only talk about Modal).

For data table, I ended up doing the former but I can see value either way 👍

@emyarod
Copy link
Member Author

emyarod commented Sep 14, 2020

yeah I can make the ComposedModal docs more scoped to ComposedModal usage. there will be some overlap at the end of the day though since they are both modals

Copy link
Collaborator

@tw15egan tw15egan 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 an awesome addition to the Modal documentation! Very thorough and will really help or users when dealing with this component. Thanks for taking the time to explain some of the more complicated areas of this component.

The only changes I have are some formatting changes to fit better with our other mdx files, better syntax highlighting, and a few wording changes (feel free to ignore those, though!). I like the usage of the InlineNotification, we just need to import it into the mdx file 👍

packages/react/src/components/Modal/Modal.mdx Outdated Show resolved Hide resolved
packages/react/src/components/Modal/Modal.mdx Outdated Show resolved Hide resolved
packages/react/src/components/Modal/Modal.mdx Outdated Show resolved Hide resolved
packages/react/src/components/Modal/Modal.mdx Outdated Show resolved Hide resolved
packages/react/src/components/Modal/Modal.mdx Outdated Show resolved Hide resolved
packages/react/src/components/Modal/Modal.mdx Outdated Show resolved Hide resolved
packages/react/src/components/Modal/Modal.mdx Outdated Show resolved Hide resolved
packages/react/src/components/Modal/Modal.mdx Outdated Show resolved Hide resolved
packages/react/src/components/Modal/Modal.mdx Show resolved Hide resolved
packages/react/src/components/Modal/Modal.mdx Show resolved Hide resolved
Copy link
Contributor

@joshblack joshblack 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 with TJ's review comments above 👀 Let me know when they're addressed and I can re-review!

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

Just a few tiny comments, other than that it looks 💯

packages/react/src/components/Modal/Modal.mdx Outdated Show resolved Hide resolved
packages/react/src/components/Modal/Modal.mdx Outdated Show resolved Hide resolved
packages/react/src/components/Modal/Modal.mdx Outdated Show resolved Hide resolved
@kodiakhq kodiakhq bot merged commit 6363d3d into carbon-design-system:master Sep 28, 2020
@emyarod emyarod deleted the 6591-modal-docs branch September 29, 2020 15:10
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.

4 participants