Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Jan 26, 2017

Added code to be compatible with react-chartist-plugin-accessibility.

@ghost ghost changed the title Allow child components and give them access to graph props. Allow child components and give them access to chartist props Jan 27, 2017
Copy link
Owner

@fraserxu fraserxu left a comment

Choose a reason for hiding this comment

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

👍

index.js Outdated
const { className, style, children, data, type } = this.props;
return (
<div className={`ct-chart ${className || ''}`} ref='chart' style={style}>
{children && cloneElement(children, { type, data })}
Copy link
Owner

@fraserxu fraserxu Jan 28, 2017

Choose a reason for hiding this comment

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

are you able to just use {this.props.children} or it's because you need to pass type and data down?

Copy link
Author

Choose a reason for hiding this comment

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

Yep! I had to use cloneElement to pass down the props to the children.

Copy link
Owner

Choose a reason for hiding this comment

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

I only used cloneElement method once when I have to passing props down to children with react-router, I've learned from others example it might be better to do a map function over the children in case user passing more than one child.

const children = React.Children.map(this.props.children, child => (
  React.cloneElement(child, {
    type,
    data
  })
))

return (
  <div>{children}</div>
)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that would work a lot better, especially if more plugins are added later.

@fraserxu fraserxu merged commit 853418c into fraserxu:master Jan 30, 2017
@fraserxu
Copy link
Owner

Thanks @theslip , new version has published to v0.12.0

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.

1 participant