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

Composition: Rename disabled parameter => disable #12603

Merged
merged 2 commits into from Sep 30, 2020

Conversation

g1eny0ung
Copy link
Contributor

@g1eny0ung g1eny0ung commented Sep 29, 2020

Issue:

https://storybook.js.org/docs/react/workflows/package-composition#configuring

After checking the source: https://github.com/storybookjs/storybook/blob/next/lib/core/src/server/manager/manager-config.js#L81, this should be a typo.

What I did

Change disable to disabled.

Deprecate the disabled parameter.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@shilman
Copy link
Member

shilman commented Sep 29, 2020

@g1eny0ung thanks for putting this PR together! 🙏

@g1eny0ung @ndelangen I think this is a bug in the code, not in the documentation. We use disable everywhere else and should be consistent.

@ndelangen
Copy link
Member

@shilman I agree. I doubt a lot of people will be affected if we change this in a minor/patch release. Do we risk it?

@g1eny0ung
Copy link
Contributor Author

g1eny0ung commented Sep 29, 2020

@shilman I also agree with u. But like @ndelangen says, changing the source code is likely to affect many people.

So I'm not sure, should I update the pr to change the source or still keep the current doc changes?

@shilman
Copy link
Member

shilman commented Sep 30, 2020

How about we support both but issue a deprecation warning if users use disabled?

Here's an example of how we do that for docs.storyDescription (old) and docs.description.story (new):

https://github.com/storybookjs/storybook/blob/next/addons/docs/src/blocks/DocsStory.tsx#L29-L35

@g1eny0ung
Copy link
Contributor Author

How about we support both but issue a deprecation warning if users use disabled?

Here's an example of how we do that for docs.storyDescription (old) and docs.description.story (new):

https://github.com/storybookjs/storybook/blob/next/addons/docs/src/blocks/DocsStory.tsx#L29-L35

Good idea. I will update the changes later.

@g1eny0ung g1eny0ung changed the title fix: doc snippet of disable refs fix: deprecated package-composition disabled parameter Sep 30, 2020
@g1eny0ung
Copy link
Contributor Author

g1eny0ung commented Sep 30, 2020

@shilman PTAL. Also, I tried to update the migration doc. If there is something wrong, please help me to point out. Very thx.~

Copy link
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.

Great work @g1eny0ung 💯

@shilman shilman changed the title fix: deprecated package-composition disabled parameter Composition: Rename disabled parameter => disable Sep 30, 2020
@shilman shilman merged commit 4f5ab9f into storybookjs:next Sep 30, 2020
@g1eny0ung g1eny0ung deleted the fix/doc-disbale-refs branch September 30, 2020 10:59
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.

None yet

3 participants