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

feat: add annotations support for addons #612

Merged
merged 17 commits into from
Nov 1, 2024
Merged

Conversation

onmotion
Copy link
Contributor

@onmotion onmotion commented Oct 1, 2024

Issue:

What I did

Added support for preview files if they are present in the addon module

I required support for the storybook-addon-deep-controls addon, but it wasn't compatible with the React Native version of Storybook. This pull request will introduce support for this module and potentially others. Specifically for this addon, I've added a preview file to the root directory as it was previously only available in the dist directory. This modification will ensure compatibility and provide a foundation for integrating other modules in the future

Screenshot 2024-10-01 at 17 29 37
Deep Controls test Screenshot 2024-10-20 at 17 23 21 Screenshot 2024-10-20 at 17 23 46
## How to test
  1. Add the addon which includes a preview enhancer
  2. Check the generated content
Screenshot 2024-10-01 at 17 04 02
  • Does this need a new example in examples/expo-example? no
  • Does this need an update to the documentation? no

@dannyhw
Copy link
Member

dannyhw commented Oct 2, 2024

Thanks for your contribution 🙏.

I'm not at home but I will give a try soon. Probably this can go in a v8 prerelease shortly.

}
const isPreviewFileExists = getPreviewExists({ configPath: addonPath });
if (isPreviewFileExists) {
enhancer.push(`require('${addon}/dist/preview')`);
Copy link
Member

@dannyhw dannyhw Oct 2, 2024

Choose a reason for hiding this comment

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

Seems like this would be without dist in the path otherwise the check would be wrong. Since getPreviewExists is checking for the root of the package from what I can see

Also we can have @storybook/addon-ondevice-actions re-export the actions preview in @storybook/addon-actions/preview so that we can follow that pattern.

enhancer.push(`require('${addon}/preview')`);

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 think if we remove the /dist path, the bundler will get the raw code since node_modules is ignored. Plus, the source file can be anywhere, but according to the docs, it should be bundled into the dist directory:
https://storybook.js.org/docs/addons/writing-addons#module-metadata But probably, we could look for a file in the dist directly.

I love the idea of re-exporting @storybook/addon-actions/preview. I just wanted to focus on the specific issue I'm facing here and don't touch anything else

Copy link
Member

@dannyhw dannyhw Oct 3, 2024

Choose a reason for hiding this comment

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

Well if this is the case then the check should be changed to look for the dist folder

I.e getpreviewpath(addonpath/dist)

Copy link
Member

Choose a reason for hiding this comment

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

I guess you already updated it but i will also check how its handled in storybook core before moving forward

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well if this is the case then the check should be changed to look for the dist folder

I.e getpreviewpath(addonpath/dist)

It won't work as the package is resolving to the /dist path. Try to play with it a little pls And let me know how to proceed

Copy link
Member

@dannyhw dannyhw Oct 3, 2024

Choose a reason for hiding this comment

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

I believe that if the exports field from package.json is used in the package like in the docs here

  "exports": {
    ".": {
      "types": "./dist/index.d.ts",
      "node": "./dist/index.js",
      "require": "./dist/index.js",
      "import": "./dist/index.mjs"
    },
    "./manager": "./dist/manager.mjs",
    "./preview": "./dist/preview.mjs",
    "./package.json": "./package.json"
  },

then this allows the import from package-name/preview assuming that package exports are enabled for metro which they are required to be for rn storybook 8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, so now should work as expected

@dannyhw
Copy link
Member

dannyhw commented Oct 3, 2024

I think it would be really cool to add deep controls to the example app so we can have an example of this being used in the project, what do you think?

@onmotion
Copy link
Contributor Author

onmotion commented Oct 3, 2024

I think it would be really cool to add deep controls to the example app so we can have an example of this being used in the project, what do you think?

Guess it's nice to have as it's pretty hard to modify jsons on mobile. Let's wait for the PR eliasm307/storybook-addon-deep-controls#24 to be merged

@dannyhw dannyhw changed the title feat: Add preview support feat: add annoations support for addons Oct 7, 2024
@dannyhw dannyhw changed the title feat: add annoations support for addons feat: add annotations support for addons Oct 7, 2024
@onmotion

This comment was marked as resolved.

@onmotion onmotion requested a review from dannyhw October 21, 2024 09:39
@dannyhw
Copy link
Member

dannyhw commented Oct 26, 2024

There's some changes we can make to simplify things, I can see that storybook-addon-deep-controls added an empty register file for react native but we can actually make that not required anymore

Also storybook-addon-deep-controls needs to define its exports in the package.json otherwise it won't resolve the preview or register file properly anyway

@onmotion
Copy link
Contributor Author

Also storybook-addon-deep-controls needs to define its exports in the package.json otherwise it won't resolve the preview or register file properly anyway

I guess it worked for me as it is

@eliasm307
Copy link

There's some changes we can make to simplify things, I can see that storybook-addon-deep-controls added an empty register file for react native but we can actually make that not required anymore

Also storybook-addon-deep-controls needs to define its exports in the package.json otherwise it won't resolve the preview or register file properly anyway

Hey @dannyhw can you clarify what you mean?

I had added exports for these with the previous release to accommodate @onmotion 's changes

Screenshot_20241026_124542_GitHub.jpg

@dannyhw
Copy link
Member

dannyhw commented Oct 26, 2024

@onmotion @eliasm307 heres a pr I made that seems to fix the issues

eliasm307/storybook-addon-deep-controls#30

  1. export name should be /preview doesn't need to have .js, otherwise I have to specify .js when importing
  2. the mjs file wasn't being included in the package so it would error

@eliasm307
Copy link

@onmotion @eliasm307 heres a pr I made that seems to fix the issues

eliasm307/storybook-addon-deep-controls#30

  1. export name should be /preview doesn't need to have .js, otherwise I have to specify .js when importing
  2. the mjs file wasn't being included in the package so it would error

@dannyhw your changes have been applied in v0.9.0

@dannyhw
Copy link
Member

dannyhw commented Nov 1, 2024

all good now, thanks for all your work on this 🙏

@dannyhw dannyhw merged commit 467b40b into storybookjs:next Nov 1, 2024
1 check passed
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.

4 participants