Skip to content

Docs: Exclude decorators by default from source#21722

Merged
tmeasday merged 8 commits into
nextfrom
tom/21649-change-excludeDecorators-true
Mar 28, 2023
Merged

Docs: Exclude decorators by default from source#21722
tmeasday merged 8 commits into
nextfrom
tom/21649-change-excludeDecorators-true

Conversation

@tmeasday
Copy link
Copy Markdown
Member

@tmeasday tmeasday commented Mar 22, 2023

What I did

Due to reordering in #21182 the source decorator comes before any user decorator in preview.js. This means that we commonly trigger the bug in #21649.

Really it was a bug that was fixed by #21182 that the user's decorators weren't visible in the source by default given we weren't excluding them.

So it makes more sense to default this to true and people can opt out in cases where they want to show the decorator.

How to test

Add the following to a React sandbox's preview.tsx:

  decorators: [
    (S) => <div><S/></div>
  ]

Then check the source block of a story:

image

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

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

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

Due to reordering in #21182 the source decorator comes *before* any user decorator in `preview.js`. This means that we commonly trigger the bug in #21649.

Really it makes more sense to default this to true and people can opt out in cases where they want to show the decorator.
@shilman shilman changed the title Docs: Exclude decorators by default from source. Docs: Exclude decorators by default from source Mar 22, 2023
@tmeasday
Copy link
Copy Markdown
Member Author

We also might want to consider doing this for React only, as I think that's the only place the issue likely happens (see #21649 (comment)).

However, I think it's reasonable to exclude decorators by default everywhere.

Copy link
Copy Markdown
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Aside from some markdown issues LGTM. Thanks @tmeasday !

Comment thread MIGRATION.md Outdated
@tmeasday tmeasday added the ci:daily Run the CI jobs that normally run in the daily job. label Mar 22, 2023
Comment thread MIGRATION.md Outdated
Comment thread MIGRATION.md Outdated
@shilman shilman dismissed their stale review March 22, 2023 10:23

Solved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

addon: docs block: source bug ci:daily Run the CI jobs that normally run in the daily job.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants