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

Chart.defaultProps.height/width shadows options.height/width #75

Closed
dkwingsmt opened this issue Oct 3, 2016 · 3 comments
Closed

Chart.defaultProps.height/width shadows options.height/width #75

dkwingsmt opened this issue Oct 3, 2016 · 3 comments

Comments

@dkwingsmt
Copy link

This is the code to compute the div style at https://github.com/RakanNimer/react-google-charts/blob/master/src/components/Chart.js#L273

    const divStyle = {
      height: this.props.height || this.props.options.height,
      width: this.props.width || this.props.options.width,
    };

I assume its purpose is "when this.props.height is absent, fallback to this.props.options.height".

But then we have defaultProps from https://github.com/RakanNimer/react-google-charts/blob/master/src/components/Chart.js#L324 which says

Chart.defaultProps = {
  width: '400px',
  height: '300px',
};

which provides this.props.height already when it's absent, and that || will never take effect unless this.props.height is manually set to null of 0 etc, and leaving height to be always 300px if we set options.height to some value without setting props.height, making the behaviour very confusing

@dkwingsmt dkwingsmt changed the title Chart.defaultProps.height/width preventing options.height/width Chart.defaultProps.height/width shadows options.height/width Oct 3, 2016
@rakannimer
Copy link
Owner

Hey !
Very good point thanks for bringing that up !
Having two ways to set width and height is not a good idea anyway, We need to find a way to set it from one place only without breaking backwards compatibility.

Do you feel like doing a PR for this ?

If not I'll get to it later this week and report back here :)

Cheers !

@dkwingsmt
Copy link
Author

I'd really love to, however since I've only started using this library recently (like a few days), I probably can not fully understand what is the best interface for it. I guess it's best to leave it to the author atm :)

Thank you for your response!

@stale
Copy link

stale bot commented Jul 22, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants