-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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: For uncontrolled scenarios, update value when defaultValue changes #5903
TextField: For uncontrolled scenarios, update value when defaultValue changes #5903
Conversation
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.
Few comments on the tests. I'll approve anyhow, so you can decide what you want to do with them ;)
|
||
textField.setProps({ value: testValue, defaultValue: defaultValue2 }); | ||
expect(textField.getDOMNode().querySelector('input')).toBeTruthy(); | ||
expect(textField.getDOMNode().querySelector('input')!.value).toEqual(testValue); |
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 know pulling out values that are used multiple times as a dev seems to do the right thing to do. Personally when it comes to test code i find it easier to read when just repeating the values.
textField.setProps({ value: 'John', defaultValue: 'Your name' });
expect(textField.getDOMNode().querySelector('input')).toBeTruthy();
expect(textField.getDOMNode().querySelector('input')!.value).toEqual('John');
Additionally it improves readability a bit more even when you put a new line between the stuff you do and the things you assert.
In the case above the setProps
and expect
.
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.
Related to my other comment, I feel this is basically a test to say input
is non-null && input
is John
before accessing input
, not two separate tests.
expect(textField.getDOMNode().querySelector('input')!.value).toEqual(testValue); | ||
|
||
textField.setProps({ defaultValue: undefined }); | ||
expect(textField.getDOMNode().querySelector('input')).toBeTruthy(); |
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 think we can probably remove these .toBeTruthy()
asserts. I'm not sure how they add value? We're basically testing querySelector
in JSDom, which should not be our responsibility :) The test would fail on the next line anyhow if it does not exist.
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 was the convention throughout this file and I copied a test, but I'm all for standardizing on test methodology. However, it's not testing for the existence of querySelector
, it's testing for the existence of the input
field. Since the next line is assuming input
is non-null, this seems it would generate clearer failure output if things are not as expected.
Pull request checklist
$ npm run change
Description of changes
For uncontrolled scenarios (when
value
prop isundefined
), updatevalue
whendefaultValue
prop changes.Focus areas to test
Add tests to make sure
value
is updated (or not) depending on controlled and uncontrolled scenarios.Microsoft Reviewers: Open in CodeFlow