Add paste handling to #39303 #40168
Closed
Add paste handling to #39303 #40168
Conversation
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
Co-authored-by: Marco Ciampini <[email protected]>
Co-authored-by: Aaron Robertshaw <[email protected]>
stokesman
commented
Apr 7, 2022
Comment on lines
-203
to
+250
| type={ isPressEnterToChange ? 'text' : 'number' } | ||
| type="text" |
Contributor
Author
There was a problem hiding this comment.
To support conventional behavior of pasting at the caret or selection the type of the input had to change to text. A number type input does not provide selectionStart/selectionEnd.
If there were a strong reason to keep the type=number the paste handling could be simplified to replace the whole input value anytime a unit is included in the paste. It might even be more convenient that way but you'd have to discover that behavior and it's unconventional.
Contributor
There was a problem hiding this comment.
From the initial conversation around this change, I initially though that we were considering to change the type to always be number in order to enjoy native browser validation.
This PR is taking the opposite approach (always set the type to text) — I'm just wondering if this change is going to impact the way users interact with the component (since the native browser validation is never going to be enforced).
If we decide to go down this route, I agree with @stokesman and @mirka that we'd really need to be careful with how the new UX works, and add as many unit tests as possible.
Contributor
There was a problem hiding this comment.
If there were a strong reason to keep the
type=numberthe paste handling could be simplified to replace the whole input value anytime a unit is included in the paste. It might even be more convenient that way but you'd have to discover that behavior and it's unconventional.
Would it be hard to experiment with always having the input being type="number" ? Would that also mean that we'd need to remove the isPressEnterToChange functionality?
|
Size Change: +222 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
stokesman
force-pushed
the
try/unit-control-alt-unit-switching-with-paste
branch
from
d26feec to
b313d11
Compare
mirka
reviewed
Apr 14, 2022
Member
mirka
left a comment
mirka
left a comment
There was a problem hiding this comment.
Really great attention to user interaction details! With sufficient test coverage, the added complexity seems acceptable to me.
| units | ||
| ); | ||
| // If no unit parsed, returns early to let the default paste happen. | ||
| if ( ! parsed[ 1 ] ) return; |
Member
There was a problem hiding this comment.
Nit: This is really Prettier territory so just my two cents, but I find it hard to scan when single-line early return statements don't have an empty line below it. It's pretty easy to misread as the beginning of an if block on a quick glance!
Comment on lines
209
to
216
| onChangeProp?.( `${ value }${ unitPasted }`, changeProps ); | ||
| } | ||
| // Moves focus to the UnitSelectControl. | ||
| refInputSuffix.current?.focus(); | ||
|
|
||
| if ( unitPasted === parsedUnit ) return; | ||
| onUnitChange?.( unitPasted, changeProps ); | ||
| setUnit( unitPasted ); |
Member
There was a problem hiding this comment.
Is it possible to do the onChangeProp/onUnitChange/setUnit calls via the designated handlers (handleOnQuantityChange/handleOnUnitChange) or do they need to be called "à la carte" like this?
| refInputSuffix.current?.focus(); | ||
| } | ||
| }; | ||
| onPaste = ( event: ClipboardEvent< HTMLInputElement > ) => { |
Member
There was a problem hiding this comment.
It might be worth extracting this as a helper function with dependency injection so it's easier to unit test the logic.
ciampo
reviewed
Apr 14, 2022
| } | ||
| }; | ||
| onPaste = ( event: ClipboardEvent< HTMLInputElement > ) => { | ||
| props.onPaste?.( event ); |
Contributor
There was a problem hiding this comment.
Just a thought that I don't think we never discussed before — should we call props.onPaste at the start or at the end of the internal callback? The same reasoning should probably be applied to other callbacks (e.g. onChange)
ciampo
reviewed
Apr 14, 2022
| // If no unit parsed, returns early to let the default paste happen. | ||
| if ( ! parsed[ 1 ] ) return; | ||
| const [ quantityPasted, unitPasted ] = parsed; | ||
| event.preventDefault(); |
Contributor
There was a problem hiding this comment.
Out of curiosity, what would happen if we didn't preventDefault() ?
ciampo
reviewed
Apr 14, 2022
Comment on lines
177
to
180
| const parsed = parseQuantityAndUnitFromRawValue( | ||
| pastedText, | ||
| units | ||
| ); |
Contributor
There was a problem hiding this comment.
Do we check at all if the pasted unit is in the list of allowed units?
And what happens if it's not an allowed unit? Maybe we should adopt the same strategy used to parse the value ?
stokesman
force-pushed
the
try/unit-control-alt-unit-switching
branch
2 times, most recently
from
2f66c6f to
8069575
Compare
stokesman
force-pushed
the
try/unit-control-alt-unit-switching
branch
from
929d872 to
86c1ccb
Compare
ciampo
force-pushed
the
try/unit-control-alt-unit-switching
branch
2 times, most recently
from
009d7c2 to
6a96ecd
Compare
Contributor
Author
|
There might be something to glean here if demand for unit-switching from pastes in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Drafted because
some tests need fixed andwe probably want to add new tests to cover the paste handling.What?
Enables changing the unit of
UnitControlby pasting a string with units into its text input. In trunk it's already possible ifisPressEnterToChangeistrueand this PR enables it regardless of that prop. Builds on #39303.Why?
In #39303, the proposed revamp of a convenience feature had the undesired effect of removing the ability to change the unit by pasting. This restores it.
How?
Adds an
onPastehandler to the text input that parses the pasted string and handles updating the quantity and unit if a valid unit was contained in the pasted string.Testing Instructions
3%,2vw,1rem.111in the input.Screenshots or screencast
TBD