-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Fix semi broken __docgenInfo integration in addon info #1030
Conversation
…omponent, fixed issue with `content` was being rendered as an object and give errors for that, added a fix to when you use `PropTypes` from something other than `React` that you can get the type from docgen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR!
_getComponentDescription() { | ||
let retDiv = null; | ||
|
||
if (Object.keys(window.STORYBOOK_REACT_CLASSES).length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the usage of window here, and why this is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ndelangen my linter gave syntax errors because it did not know where STORYBOOK_REACT_CLASSES
came from. I understand that STORYBOOK_REACT_CLASSES
is a global variable and that should work without the need of window
at all. Sorry for any confusion I really only added it to get rid of the errors from my linter. I would be more than willing to do a commit that removes the need for window
.
Hey @bigassdragon, I was actually going to open a PR to add these features as well. Thanks for doing this. You should probably run your code through the project's eslint config and rebase off the latest master. A lot of lines in your changes are stylistic but it doesn't match the convention used across the project. That's why one of the bitHound tests is failing. I'd love to see this merged soon, let me know if you need help. |
@danielduan Thank you for your comment, I was a little confused. I ran it through eslint using the config inside of storybook and committed the code that I modified. |
@bigassdragon you still need to do a rebase unfortunately. there are conflicts which means that there have been changes to the files since you last edited them so your changes can't be automatically merged. |
…omponent, fixed issue with `content` was being rendered as an object and give errors for that, added a fix to when you use `PropTypes` from something other than `React` that you can get the type from docgen.
@danielduan I have rebased now and committed. |
package.json
Outdated
@@ -28,6 +28,7 @@ | |||
}, | |||
"scripts": { | |||
"bootstrap": "lerna bootstrap", | |||
"postinstall": "lerna bootstrap --hoist", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because a few reason we don't do this, and instead bootstrap manually.
- For some CI tasks bootstrapping is unnecessary, but installing is. Bootstrapping is rather slow.
- We do npm install in examples, this will cause the postinstall to re-run, causing a lot of unnecessary delay.
- When adding a root dependency for development, bootstrapping is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this postinstall hook?
package.json
Outdated
@@ -28,7 +28,6 @@ | |||
}, | |||
"scripts": { | |||
"bootstrap": "lerna bootstrap", | |||
"postinstall": "lerna bootstrap --hoist", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah never mind then
package.json
Outdated
@@ -1,6 +1,5 @@ | |||
{ | |||
"name": "storybook", | |||
"version": "1.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused 🙄
addons/info/src/components/Story.js
Outdated
import Node from './Node'; | ||
import { baseFonts } from './theme'; | ||
import { Pre } from './markdown'; | ||
import PropTypes from 'prop-types' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, you linted everything right, and I made you revert it... Luckely linting will soon be fixed! #1000
Hey @bigassdragon, Thanks you so much, quite a lot of work you did here, I'll be releasing a new alpha, I really hope you can help test this? 🏅 |
Hey @ndelangen, You are very welcome, sorry for taking so long. I would be more than happy to test it once it is available. I hope to make more PR's in the future, I really like what you guys are doing here. Thanks |
Hey @bigassdragon, thanks for this. Might need your help updating the addon-info README as well. I personally haven't been able to get the __docgenInfo on the components so I'd love to see what you're doing to the |
Hey @danielduan I am not actually doing anything special to get __docgenInfo running on my side at all. I did however add |
It's published! |
@ndelangen awesome thank you very much! |
@ndelangen I am unable to download the new version. Maybe I am doing something wrong. I tried this |
Yeah that's incorrect, package name is |
Did it work for you @bigassdragon ? |
Hey @ndelangen, I have been busy with personal life and work, so I have not had time to try everything out yet sorry. I will make some time to try it all out today. Thanks for the reminder. |
Hey @ndelangen, I double checked everything, and it looks like everything is a drop in fix for it. I am able to use all the docgen information without any issues. Thanks again for taking this PR. |
Yay! Thanks for your contribution @bigassdragon 🙇 BTW any thoughts on #1147? |
Hey @shilman, Would that be a new addon? |
Does this still work? I worked on this with @bigassdragon when he originally fixed it, but I can't seem to get it to work in my current project using these versions:
I added this to config.js: window.STORYBOOK_REACT_CLASSES = {} and have comments like this: MyComponent.propTypes = {
/**
* number of stars to display full
*/
stars: PropTypes.number,
/** color to use for fill and stroke */
color: PropTypes.string
} In my stories, |
@konsumer Can you open a new issue? |
@ndelangen Yep, no prob. #1762 |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit d08b51b. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
Issue:
I'd like to see the component description from __docgenInfo in the story documentation in the info plugin.
I'd like to get the prop type from __docgenInfo for when the type comes back as "other".
PropVal had a problem where 'content' was being directly rendered as an object.
What I did
added to PropTable
added to PropVal
added to Story
How to test
Make a story with a component that has documented (via docgen-style text) description, and the description will show up.
Like this:
You will see a description under the info description, you will also see that the prop type is not 'other' when you use PropTypes from something other than React (for support of React >= 15.5.x.