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

Add initialHeight and initialWidth to withParentSize #836

Merged
merged 3 commits into from
Oct 6, 2020

Conversation

shannonrothe
Copy link
Contributor

@shannonrothe shannonrothe commented Oct 2, 2020

🚀 Enhancements

Adds the ability to set initialWidth/initialHeight to @visx/responsives withParentSize. Closes #554.

@coveralls
Copy link

coveralls commented Oct 2, 2020

Pull Request Test Coverage Report for Build 77

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 55.296%

Totals Coverage Status
Change from base Build 283122090: 0.03%
Covered Lines: 1153
Relevant Lines: 2029

💛 - Coveralls

Copy link
Contributor

@wyze wyze left a comment

Choose a reason for hiding this comment

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

In my opinion, this change should be reverted. It is an anti-pattern as you can see here https://reactjs.org/docs/react-component.html#constructor if you scroll down a little bit to the Note section.

A better solution would be to modify the render function like so:

  render() {
+   const { initialWidth, initialHeight } = this.props;
+   const { parentWidth = initialWidth, parentHeight = initialHeight } = this.state;
-   const { parentWidth, parentHeight } = this.state;
    return (
      <div style={CONTAINER_STYLES} ref={this.setRef}>

@williaster
Copy link
Collaborator

thanks for the PR @shannonrothe!

I'm okay with this implementation or @wyze 's suggestion. The react docs highlight that it's an anti-pattern if you are expecting state to update when props change and you get into a mess with componenDidUpdate, but here the naming of the props with initialXXX makes is clear that's not the expectation.

Let me know which you prefer and we can merge this puppy 🚀 .

@shannonrothe
Copy link
Contributor Author

thanks for the PR @shannonrothe!

I'm okay with this implementation or @wyze 's suggestion. The react docs highlight that it's an anti-pattern if you are expecting state to update when props change and you get into a mess with componenDidUpdate, but here the naming of the props with initialXXX makes is clear that's not the expectation.

Let me know which you prefer and we can merge this puppy 🚀 .

Yeah I thought it would be implied that those values wouldn't update but I think @wyze's suggestion is a little nicer. I've fixed up so should be ready to merge 👍

Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @shannonrothe!

@williaster williaster merged commit 24a31ca into airbnb:master Oct 6, 2020
@williaster williaster added this to the 1.1.0 milestone Oct 17, 2020
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.

Feature: specify default width/height for withParentSize from @visx/responsive
5 participants