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

[TextField] sync height with shadow when rowsMax changed #6297

Merged
merged 2 commits into from
Mar 8, 2017

Conversation

seasick
Copy link
Contributor

@seasick seasick commented Mar 7, 2017

This will adjust the height of the component when rowsMax property has changed.
Currently I am using this to expand the EnhancedTextarea when the element gains focus (and reset it when it blurs).

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

@oliviertassinari oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI bug 🐛 Something doesn't work component: text field This is the name of the generic UI component, not the React module! and removed PR: Needs Review labels Mar 7, 2017
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Thanks for taking the initiative to address the issue.

@@ -75,6 +75,12 @@ class EnhancedTextarea extends Component {
}
}

componentDidUpdate(prevProps) {
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't need that hook. componentWillReceiveProps should be enough.
I think that the issue is coming from syncHeightWithShadow using the old maxHeight value and not the up coming one. Does that makes sens for you?

Copy link
Contributor Author

@seasick seasick Mar 8, 2017

Choose a reason for hiding this comment

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

Yes, the actual problem is, that syncHeightWithShadow is using this.props.rowsMax, which will contain the value before the property was changed. Thus the function also calculates the height for the old value.

This is the "culprit":

    if (this.props.rowsMax >= this.props.rows) {
      newHeight = Math.min(this.props.rowsMax * rowsHeight, newHeight);
    }

Another solution to this can be to call syncHeightWithShadow with an additional argument props and using them instead of this.props -- syncHeightWithShadow(newValue, event, props).

I think its a slightly better solution, since syncHeightWithShadow won't be called twice.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

That looks good to me 👍

@oliviertassinari oliviertassinari added PR: Review Accepted and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 8, 2017
@mbrookes mbrookes merged commit 5bcd6e7 into mui:master Mar 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: text field This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants