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

Adds react-docgen-displayname-handler #205

Merged
merged 5 commits into from
Oct 28, 2016
Merged

Adds react-docgen-displayname-handler #205

merged 5 commits into from
Oct 28, 2016

Conversation

tizmagik
Copy link
Collaborator

@tizmagik tizmagik commented Oct 24, 2016

Fixes #202

This adds react-docgen-displayname-handler to the default handlers[] config. I wasn't sure whether to include it in the default config or forcibly add it where it's used in props.loader.js. I opted to include it into the configuration to make it more extensible in userland. Feedback welcome!

This also adds a <WrappedButton /> example which shows off HoC support.

Array of functions used to process the discovered components and generate documentation objects. Default behaviours include discovering component documentation blocks, prop types, and defaults. If setting this property, it is best to build from the default `react-docgen` handler list, such as in the example below. See the [react-docgen handler documentation](https://github.com/reactjs/react-docgen#handlers) for more information about handlers.

> NOTE: By default, `react-docgen-displayname-handler` is included which provides better support for Higher Order Components. If you'd like to add additional handlers, be sure to _add_ to the Array rather than replace entirely.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now that I think about it, this isn't actually possible because we don't provide a "callback" per-say, we just take their config.handlers value.

Hm... any thoughts on how we should handle this?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I even understand this part: “be sure to add to the Array rather than replace entirely”. You mean to copy default list and add your own stuff?

Copy link
Collaborator Author

@tizmagik tizmagik Oct 24, 2016

Choose a reason for hiding this comment

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

copy default list and add your own stuff

Right, but I think there's no real way to copy the default list, right? I mean at runtime at least, sure you can look at the source and reproduce the list but that's an implementation detail that the user would have to worry about/figure out.

What do you suggest here?

Copy link
Member

@sapegin sapegin Oct 25, 2016

Choose a reason for hiding this comment

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

I’d suggest to give an example of overriding handlers array that will include react-docgen-displayname-handler so people could just copy it and change whatever they want. Instead of this note paragraph.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea! I've added that.

@sapegin
Copy link
Member

sapegin commented Oct 24, 2016

And I agree that making it default handler is a good idea. More flexibility for us, if we ever decide to add more default handlers, and for the users, who could customize handlers as they want.

* master:
  Update react-docgen.
  Update lint.
  Fix lint warning.
  Fix regression: colors in props table.
  Refactoring: simplify PropsRenderer a bit (#206)

# Conflicts:
#	package.json
@tizmagik
Copy link
Collaborator Author

Not sure why the build is failing, looks unrelated to code changes. 🤔

@sapegin
Copy link
Member

sapegin commented Oct 27, 2016

Have no idea. Looks like some issues with npm.

@sapegin
Copy link
Member

sapegin commented Oct 27, 2016

Works fine with Webpack 1.13.3, please merge master into your branch.

* master:
  Update Webpack.
@tizmagik
Copy link
Collaborator Author

@sapegin build seems to be passing now :shipit:

@sapegin sapegin merged commit 28f2331 into master Oct 28, 2016
@sapegin
Copy link
Member

sapegin commented Oct 28, 2016

Cool, thanks!

@sapegin sapegin deleted the fix-hoc branch October 28, 2016 05:49
sapegin added a commit that referenced this pull request Oct 28, 2016
We still need react-docgen default handlers, not just displayName
handler 🚑
@chrisdrackett
Copy link

chrisdrackett commented Nov 4, 2016

I put the handlers code in my config and I'm now getting a SyntaxError: Unexpected token )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants