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: Add Writing Stories in TypeScript docs guide #23711

Merged
merged 3 commits into from
Aug 20, 2023

Conversation

kylegach
Copy link
Contributor

@kylegach kylegach commented Aug 3, 2023

Closes #22595, closes #23362

What I did

  • Add Writing Stories in TypeScript (/writing-stories/typescript) docs guide
  • Use same snippets for all "custom args" examples
  • Add/update/remove snippets

How to test

  1. Follow the steps in the contributing instructions for this branch, writing-stories-in-typescript

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>

@kylegach kylegach added documentation patch:yes Bugfix & documentation PR that need to be picked to main branch ci:docs Run the CI jobs for documentation checks only. labels Aug 3, 2023
@kylegach kylegach self-assigned this Aug 3, 2023
@kylegach kylegach force-pushed the writing-stories-in-typescript branch from ebcac82 to 9c87ffb Compare August 3, 2023 16:25
@kylegach kylegach force-pushed the writing-stories-in-typescript branch from 9c87ffb to 78df445 Compare August 4, 2023 17:09
@kasperpeulen kasperpeulen removed the patch:yes Bugfix & documentation PR that need to be picked to main branch label Aug 9, 2023
@kylegach kylegach force-pushed the writing-stories-in-typescript branch from 78df445 to e7f654a Compare August 10, 2023 20:46
@kylegach kylegach marked this pull request as ready for review August 10, 2023 20:48
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.

Appreciate you taking the time to put together this great addition to the documentation, and can't wait to get it out and made available to our readers. Left a couple of items for you to look at when you have a chance.

Let me know and we'll go from there.

docs/writing-stories/typescript.md Outdated Show resolved Hide resolved
Comment on lines 39 to 50
<div class="aside">

ℹ️ We are not yet able to provide additional type safety using the `satisfies` operator with Angular and Web components.

<details>
<summary>More info</summary>

Both Angular and Web components utilize a class plus decorator approach. The decorators provide runtime metadata, but do not offer metadata at compile time.

As a result, it appears impossible to determine if a property in the class is a required property or an optional property (but non-nullable due to a default value) or a non-nullable internal state variable.

For more information, please refer to [this discussion](github.com/storybookjs/storybook/discussions/20988).

</details>
Copy link
Contributor

Choose a reason for hiding this comment

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

@kylegach while previewing this locally, it reads a bit weird and adds a bit of confusion with the amount of information you're trying to present. A couple of options we could here:
1- Condense the information in a single paragraph without the aside and provide the link for the relevant discussion (don't forget to update the link as it throws a 404).
2 - Push this aside into a troubleshooting section visible only for the frameworks (i.e., Angular and Web Components) with some word adjustments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed the link (thanks for the catch!). But I still like the current approach better than either of those options.

  • The aside makes it immediately clear to the affected users that satisfies is not available.
  • Rather than just taking our word for it, the "More info" provides the why.
    • While this could be an anchor link to a conditionally rendered troubleshooting section, that seemed far more clunky than a details disclosure.
  • I don't think we should combine the initial message with what's in "More info", because that is too much information to present upfront.


## Framework specific tips

Template-based frameworks such as Vue and Svelte typically require editor extensions to enable syntax highlighting, autocomplete, and type checking. Here are a few tips to help you set up the ideal environment for them.
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind taking a second pass at this and restructuring the documentation a bit, as this is going to be applied only for Vue and Svelte; it seems that the heading seems a bit redundant when a user is already previewing the documentation for any of those frameworks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the push toward simplicity!


<!-- prettier-ignore-end -->

### Optional type parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate on the need for this heading when the text within it relates to the above? Couldn't it be removed and the text below adjusted to provide the information explicitly? More even as we're teaching users to use this pattern in all the examples we provide, and adding this may add some confusion simply by referencing it as optional.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When structuring docs, I try to think about what would be useful to link to later, as a reference. And an explanation of the type parameter would be a useful reference to provide for many of the conversations I've had.

That said, I appreciate your suggestion that the "Optional" part is emphasized too much. I tried to address that in my update.

title: 'Writing stories in TypeScript'
---

Writing your stories in [TypeScript](https://www.typescriptlang.org/) makes you more productive. You don't have to jump between files to look up component props. Your code editor will alert you about missing required props and even autocomplete prop values. Plus, Storybook infers those component types to auto-generate the [Controls](../api/doc-block-controls.md) table.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you take another pass at the introduction and condense it a bit to be more straight to the point, as the way it's written, it seems that we're repeating the same idea.

@kylegach kylegach changed the title Add Writing Stories in TypeScript docs guide Docs: Add Writing Stories in TypeScript docs guide Aug 15, 2023
@kylegach kylegach force-pushed the writing-stories-in-typescript branch 2 times, most recently from b64895e to b9441b9 Compare August 15, 2023 19:28
- Use same snippets for all "custom args" examples
- Add/update/remove snippets
- Simplify introduction
- Fix links
- Tweak "type parameter" heading & text
- Fix typos
- Simplify renderer-specific content structure
@kylegach kylegach force-pushed the writing-stories-in-typescript branch from b9441b9 to dbde31b Compare August 20, 2023 14:52
@kylegach kylegach merged commit d7291c7 into next Aug 20, 2023
15 checks passed
@kylegach kylegach deleted the writing-stories-in-typescript branch August 20, 2023 15:15
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.

Fully custom args example has type error Add TypeScript guide under Write stories section
3 participants