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

Clean up Svelte snippets + Create Svelte/Typescript snippets #23658

Closed
wants to merge 10 commits into from
Closed

Clean up Svelte snippets + Create Svelte/Typescript snippets #23658

wants to merge 10 commits into from

Conversation

jack-weilage
Copy link

@jack-weilage jack-weilage commented Jul 31, 2023

What I did

  1. Removed .native-format.mdx (Re-added) and .mdx.mdx snippets for Svelte
  2. Removed unreferenced snippets
  3. Updated Svelte/JS snippets and created new Svelte/Typescript snippets, staying close to React/Typescript snippets

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

@kasperpeulen kasperpeulen added documentation ci:docs Run the CI jobs for documentation checks only. labels Jul 31, 2023
Copy link
Contributor

@jonniebigodes jonniebigodes left a comment

Choose a reason for hiding this comment

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

@jack-weilage, thanks for taking the time to put together this pull request and help us improve our documentation by introducing the TypeScript variations of the Svelte scripts and fixing the other ones. However, there are a couple of things before this can move forward. If you're ok with it, can you re-introduce the .native-format.mdx examples but don't link them? The reasoning behind this is that there's some work being done on that front with the Svelte native addon, and we want to introduce them at some point. Also, it seems that there are some issues in the examples, and once again, if you're ok with it, could you run yarn pretty-docs to fix them?

With that in mind @JReinhold, if memory serves me right, you've been doing some work on that addon, correct? If so, could you take a pass at this one and follow up with me so that we have a sense of the status of the addon and see when we're ready to start re-introducing them?

Let me know once you've addressed the issues so that we can follow up with you on it.

Hope you have a great day.

Stay safe

@jack-weilage
Copy link
Author

jack-weilage commented Aug 2, 2023

@jonniebigodes, When re-adding the .native-format.mdx files, should I also add the .mdx.mdx files, or should they remain removed?

@jonniebigodes
Copy link
Contributor

@jack-weilage, thanks for following up with me. That's a great question, and sorry for not being clear. And to answer it, the .mdx.mdx files not be added, only the ones I've mentioned.

@jack-weilage
Copy link
Author

That should be everything done now. Funnily enough, I had already run prettier locally, but just hadn't pushed it!

Copy link
Contributor

@jonniebigodes jonniebigodes left a comment

Choose a reason for hiding this comment

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

@jack-weilage, just one item from you, a small typo, and this should be ready. Also, when you're able, can you solve the conflicts and let me know so that we're able to get this merged?

Hope you have a fantastic weekend.

Stay safe

@@ -0,0 +1,28 @@
```ts
//MyComponent.stories.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

@jack-weilage could you add a space here, so that it aligns with the other examples?

@jonniebigodes
Copy link
Contributor

@jack-weilage just a friendly reminder to see if you have a moment to address the feedback I've mentioned and vet the conflicts so that we can get this one in.

Let me know and we'll take it from there.

Hope you have a fantastic week.

Stay safe

@JReinhold
Copy link
Contributor

this looks great @jack-weilage, awesome work! 🙏 In regards to the native Svelte format, it's still a bit unclear where that will end, but we're not ready to introduce them back yet, and if we do it will likely be with some syntax changes. so I think it's safest to just leave them as is for now.

@jonniebigodes
Copy link
Contributor

@jack-weilage it goes without saying that we really appreciate you taking the time to put this pull request together and helping us improve the documentation 🙇 . But and I hope you don't mind, I'm going to close as I've got a pull request for the TypeScript variants for Svelte including TypeScript 4.9 and some items that required some additional attention regarding how they're shown.

Looking forward to seeing your next contribution.

Hope you have a fantastic day.

Stay safe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:docs Run the CI jobs for documentation checks only. documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants