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

Fix: Docs for Blankslate Component #3623

Closed
wants to merge 5 commits into from

Conversation

electron97
Copy link
Contributor

@electron97 electron97 commented Aug 13, 2023

Closes #3620

Summary

  • Currently when visiting the React docs for Blankslate the import is incorrect.
  • Also the storybook stories do not link correctly.
  • Added stories to Blankslate.docs.json so that it appears correctly on the docs site.
  • Also added Blankslate.mdx file to fix incorrect import.

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Changes are SSR compatible
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

- Currently the import statement on the docs site is incorrect.
- Also the storybook stories is link correctly.
- Added stories in Blankslate.docs.json to fix the issue and also fixed the docs by adding Blankslate.mdx
@electron97 electron97 requested review from a team and mperrotti August 13, 2023 18:20
@changeset-bot
Copy link

changeset-bot bot commented Aug 13, 2023

⚠️ No Changeset found

Latest commit: d02790d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@electron97 electron97 temporarily deployed to github-pages August 13, 2023 18:28 — with GitHub Actions Inactive
"stories": [],
"stories": [
{
"id": "components-blankslate--default",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"id": "components-blankslate--default",
"id": "drafts-components-blankslate--default",

Blankslate is currently a draft component, so in order to ensure the Storybook embed is pulled in accurately I think this may need to be adjusted.

Copy link
Contributor Author

@electron97 electron97 Aug 15, 2023

Choose a reason for hiding this comment

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

Hey @lesliecdubs ,

Thanks for your review I have addressed your review feedback and updated the PR accordingly.
I have one question as I am new to this repo and wanted to understand how things are working.
Is this metadata being imported by some other repo to render it on the docs site because when I am running the primer/react repo locally I can only see the storybook of all the components and I was not able to test my changes which I made in Blankslate.docs.json locally.
Could you please let know whenever you get a chance?

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @electron97!

@broccolinisoup (FR) do you have the context to help answer this question about how the docs are rendered? If not I can try to do some digging.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @lesliecdubs for the ping, I think I can help. I don't have a deep knowledge on all details yet but I can provide some context.

Is this metadata being imported by some other repo to render it on the docs site

@electron97 Yes! We export the component metadata and render the docs in https://github.com/primer/design repo, specifically this line fetches the react components data. When you render the docs in react, what you will access to is only the react docs https://primer.style/react

If you can look at the Blankslate component's data on the unpkg url, the story id is components-blankslate--default and it is because we generate a default story id for each component (reference)

Adding "id": "drafts-components-blankslate--default", to the docs.json, I believe the story will be available on https://primer.style/design/components/blankslate/react - I am just not sure if the current broken story will go away 🤔 We will see!

Regardless, I think your changes looks great ~ I only have a comment about the changeset. We usually don't publish any changeset for docs work. If this is okay with you, would you remove the changeset file and I can add skip changeset label? Let me know if you have any concern though or if you need any more clarification around how the docs are rendered. I hope it was helpful! Thank you so much for your contribution 🙏🏻

Copy link
Contributor Author

@electron97 electron97 Aug 25, 2023

Choose a reason for hiding this comment

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

Hey @broccolinisoup,

Thanks for the detailed explanation it was really helpful and it would definitely help me in the future working with component metadata. I have removed the changeset and updated the PR accordingly feel free to add a skip changeset label.

I have one question initially I had created a Blankslate.mdx file which I thing is missing currently and for that reason we are not seeing any Blankslate docs here in https://primer.style/react. I didn't wanted to blocked this PR because this is fixing docs for Blankslate component. So do you think we should create a new issue for missing Blankslate docs in https://primer.style/react or do you think it was not added due to some other reasons?

Could you please let me know?
Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

So do you think we should create a new issue for missing Blankslate docs in https://primer.style/react or do you think it was not added due to some other reasons?

We are in the process of consolidating our docs in https://primer.style/design and React related docs will be on the the component's React tab. So, at this stage we are not looking into adding docs to https://primer.style/react.

Thanks so much for your contribution to fix the story id 🙏🏻

src/Blankslate/Blankslate.docs.json Outdated Show resolved Hide resolved
@electron97 electron97 temporarily deployed to github-pages August 15, 2023 09:44 — with GitHub Actions Inactive
@broccolinisoup broccolinisoup added the skip changeset This change does not need a changelog label Aug 28, 2023
@broccolinisoup
Copy link
Member

Just cross posting the comments from this PR #3670 - We will probably want to solve them in the same way

@broccolinisoup
Copy link
Member

#3670 fixes the default story id for components that are not in the main bundle (i.e. drafts, deprecated etc) With the fix is out, BlankSlate docs are now rendered correctly 🎉 I'm closing this PR but please let me know if there is any concern 🙌🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changeset This change does not need a changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs for Blankslate component are incorrect
3 participants