Conversation
ciampo
added
[Feature] UI Components
ciampo
requested review from
aaronrobertshaw,
andrewserong,
brookewp,
chad1008,
mirka,
stokesman and
tyxla
|
|
ciampo
commented
Nov 23, 2022
Comment on lines
+199
to
+227
| it( 'should parse to number value on ENTER keypress when required', async () => { | ||
| const user = userEvent.setup( { | ||
| advanceTimers: jest.advanceTimersByTime, | ||
| } ); | ||
|
|
||
| render( <NumberControl value={ 5 } required={ true } /> ); | ||
|
|
||
| const input = getInput(); | ||
| await user.clear( input ); | ||
| await user.type( input, 'abc' ); | ||
| await user.keyboard( '[Enter]' ); | ||
|
|
||
| expect( input ).toHaveValue( 0 ); | ||
| } ); | ||
|
|
||
| it( 'should parse to empty string on ENTER keypress when not required', async () => { | ||
| const user = userEvent.setup( { | ||
| advanceTimers: jest.advanceTimersByTime, | ||
| } ); | ||
|
|
||
| render( <NumberControl value={ 5 } required={ false } /> ); | ||
|
|
||
| const input = getInput(); | ||
| await user.clear( input ); | ||
| await user.type( input, 'abc' ); | ||
| await user.keyboard( '[Enter]' ); | ||
|
|
||
| // React Testing Library associated `null` to empty string types for | ||
| // numeric `input` elements | ||
| // (see https://github.com/testing-library/jest-dom/blob/v5.16.5/src/utils.js#L191-L192) | ||
| expect( input ).toHaveValue( null ); |
Contributor
Author
There was a problem hiding this comment.
Previously, in these two tests we were using fireEvent to submit a new value 10 abc — the fact that the value was set in one go meant that the whole string couldn't be parsed as a number.
Switching to user-event meant that each single character in that string would be typed individually. This change was causing the tests to fail, because the component would first parse the number 10 correctly, and then refuse to parse ' abc' — meaning that the input's value would become 10 in both tests, instead of 0 and null.
I therefore decided to change slightly the tests, and simulate the user entering a fully invalid number (ie. 'abc'), which makes the tests pass again.
Contributor
There was a problem hiding this comment.
A question related to the second of these tests: Do we expect the visible value in regular browsers to actually clear? That's what the test suggests to me but it's not the case.
number-control-test-parse-to-empty.mp4
If that's what we expect, I don't think it can actually be tested by jsdom (or even very easily in most browsers) because in number inputs non-numeric values are reported as ''. That means the second test is really only good for testing that NumberControl doesn't produce a numeric value in this scenario. I suppose there’s some value in that but maybe the test could be more clearly written that way.
I don't think it’s anything this PR should address. I just wanted to bring it up.
Contributor
Author
There was a problem hiding this comment.
Good point! I tried to make some small improvements in 6732efb, hopefully it's a step in the right direction!
ciampo
commented
Nov 23, 2022
Comment on lines
+243
to
+229
| // React Testing Library associated `null` to empty string types for | ||
| // numeric `input` elements | ||
| // (see https://github.com/testing-library/jest-dom/blob/v5.16.5/src/utils.js#L191-L192) |
Contributor
Author
There was a problem hiding this comment.
As part of the refactor, I switched from expect( input.value ).toBe( ... ) to expect( input ).toHaveValue( ... ).
This change has two consequences, caused by how Testing Library extracts the value of numeric inputs:
- the expected value is parsed as a number (previously the tests were expecting strings)
- empty strings are converted to
null
These are effectively only changes to how the assertions are made, but I wanted to clarify them since there's already enough confusion around the type of the value prop.
Member
There was a problem hiding this comment.
How about .not.toHaveValue()? If that works, I believe it will be a better representation of what we're asserting here.
Contributor
Author
There was a problem hiding this comment.
I tried that out, and realised that unfortunately .not.toHaveValue() passes for all falsey values — which means that we wouldn't have a way to tell the difference between 0 and ''.
Going to stick with the current checks for now
|
Size Change: -1 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
mirka
approved these changes
Nov 23, 2022
mirka
reviewed
Nov 23, 2022
tyxla
approved these changes
Nov 24, 2022
Member
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
Looking pretty awesome, great work!
Left a few minor suggestions, nothing blocking.
🚀
Comment on lines
+243
to
+229
| // React Testing Library associated `null` to empty string types for | ||
| // numeric `input` elements | ||
| // (see https://github.com/testing-library/jest-dom/blob/v5.16.5/src/utils.js#L191-L192) |
Member
There was a problem hiding this comment.
How about .not.toHaveValue()? If that works, I believe it will be a better representation of what we're asserting here.
ciampo
force-pushed
the
refactor/number-control-tests-stories-typescript
branch
from
f644ed5 to
f75fcd0
Compare
ciampo
force-pushed
the
refactor/number-control-tests-stories-typescript
branch
from
f75fcd0 to
98d8b1d
Compare
ciampo
force-pushed
the
refactor/number-control-tests-stories-typescript
branch
from
98d8b1d to
6732efb
Compare
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
This PR refactors to TypeScript all remaining JavaScript files in
NumberControlWhy?
@wordpress/componentspackage to TypeScript #35744NumberControl: introducestringas a type for thevalueprop #45949NumberControl: tidy up and fix types aroundvalueprop #45982How?
Most of the changes are trivial, but I'll highlight the 🌶️ bits with inline comments.
Reviewing commit-by-commit should be a much easier task
Testing Instructions