-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[core] fix(TextArea): resize vertically in controlled mode #5975
[core] fix(TextArea): resize vertically in controlled mode #5975
Conversation
@AaronJRubin I think the tests are not working because we're not mounting into an |
@AaronJRubin I wasn't able to get the tests working, but I've added a little example to the docs which demonstrates controlled usage w/ Note that it does not shrink vertically automatically, but this matches the existing behavior with the uncontrolled API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self-review
@@ -75,22 +79,29 @@ export class TextArea extends AbstractPureComponent2<TextAreaProps, ITextAreaSta | |||
this.props.inputRef, | |||
); | |||
|
|||
public componentDidMount() { | |||
if (this.props.growVertically && this.textareaElement !== null) { | |||
// HACKHACK: this should probably be done in getSnapshotBeforeUpdate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't really help us. removed this comment
public componentDidMount() { | ||
if (this.props.growVertically && this.textareaElement !== null) { | ||
// HACKHACK: this should probably be done in getSnapshotBeforeUpdate | ||
/* eslint-disable-next-line react/no-did-mount-set-state */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a significantly better way to do this, and I think the potential for layout thrashing is minimal. removed this comment.
if (prevProps.inputRef !== this.props.inputRef) { | ||
setRef(prevProps.inputRef, null); | ||
this.handleRef = refHandler(this, "textareaElement", this.props.inputRef); | ||
setRef(this.props.inputRef, this.textareaElement); | ||
} | ||
|
||
if (prevProps.value !== this.props.value || prevProps.style !== this.props.style) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that props.style
changes can cause layout changes which affect scrollHeight
Fixes #5828
Checklist
Changes proposed in this pull request:
Resize text area when its value changes due to a new string being passed in through props
Reviewers should focus on:
How we can test this - at first, I tried adding a test like the below:
But both newHeight and initialHeight are always 0, so this test fails. The existing test for the resize behavior (
can fit large initial content
) appears to be spurious; the comparison at https://github.com/palantir/blueprint/blob/develop/packages/core/test/forms/textAreaTests.tsx#L67 is also just comparing the strings0px
and0px
.I manually tested this by building another app against the locally-compiled blueprint-core package and observing that the buggy behavior was fixed, but I'm not sure if that meets our standards for testing.