feature: userEvent clear()
#1440
Conversation
mdjastrzebski
requested review from
thymikee,
AugustinLF,
MattAgn and
pierrezimmermannbam
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1440 +/- ##
==========================================
+ Coverage 97.56% 97.57% +0.01%
==========================================
Files 72 74 +2
Lines 4266 4338 +72
Branches 631 636 +5
==========================================
+ Hits 4162 4233 +71
- Misses 104 105 +1
☔ View full report in Codecov by Sentry. |
src/user-event/clear.ts
Outdated
| } | ||
|
|
||
| // Skip events if the element is disabled | ||
| if (element.props.editable === false || !isPointerEventEnabled(element)) { |
There was a problem hiding this comment.
could we want to log or throw to inform the user the action is not possible? because I think if he did it, he did not realize his component is not editable and it would be nice to notify him of it
There was a problem hiding this comment.
I think this is a good idea to throw a helpful error in such case.
There seems to be some discrepancy in OG User Event on that front:
clear()andclick()/pointer()seem to throw if element is not enabledtype()seems to silently ignore the fact.
I think we should have consistent semantics in such case, i.e. always throw a helpful error.
One thing to note, this would be again the Fire Event API which silently ignores such events.
@MattAgn @pierrezimmermannbam @AugustinLF @thymikee any thoughts on that more general subject?
There was a problem hiding this comment.
i agree, we should always throw errors and document it well so people are not too surprised it's different with FireEvent. And maybe we could also think if we want to change the behavior of fireEvent in the future
There was a problem hiding this comment.
Added throwing error here in case element is not editable nor pressable (pointer events prop).
For other types of elements, I'll open an issue for discussion.
| const previousText = element.props.value ?? element.props.defaultValue ?? ''; | ||
| const selectionRange = { | ||
| start: 0, | ||
| end: previousText.length, |
There was a problem hiding this comment.
since it starts at 0, shoud not the end be previousText.length -1 ?
There was a problem hiding this comment.
No this is actually fine, 0 is before first character, 1 is after first character, so n would be after n characters.
|
|
||
| // 3. Press backspace | ||
| const finalText = ''; | ||
| await emitTypingEvents( |
There was a problem hiding this comment.
it's funny that with the backspace event, we still have to precise the finalText where the action itself of deleting should be enough to determine the final text
There was a problem hiding this comment.
emitTypingEvents is primitive function that just emits events and does not calculate changes. It expects to receive all of following: key pressed, text after change, text before change.
There was a problem hiding this comment.
oh alright, i did not go check the implementation behind it
| @@ -79,14 +78,16 @@ export async function type( | |||
| dispatchEvent(element, 'blur', EventBuilder.Common.blur()); | |||
| } | |||
|
|
|||
| async function emitTypingEvents( | |||
| export async function emitTypingEvents( | |||
| config: UserEventConfig, | |||
There was a problem hiding this comment.
[nitpick] i often see config options passed as the last argument (usually because they're optional), which makes me wonder if we always want these options? could we want a default config ?
There was a problem hiding this comment.
This is an internal function (export is not added to user-event/index.ts), we need the current config in order to invoke wait, etc
There was a problem hiding this comment.
Very nice addition! It's nice to see userEvent API coming together!
The only thing I'm not sure is whether it should throw or not for disabled elements but we can have that discussion on the dedicated issue. However I don't really like the idea of a release where clear would throw but not type and press so APIs should be aligned before next release imo and because I'm not convinced yet that we should throw maybe the easiest for now is to keep that for another PR?
|
I've removed throwing error on disabled/not focusable element. |
|
This PR has been released in v12.2.1 🚡 |
Summary
Method for clearing the content of
TextInputelement. This method is useful astype()helper, following OG User Event implementation, is appending text and not replacing it. Hence, users need a way to clear it in some cases. Currently they have to resort tofireEvent.changeText(element, '')workaround.Test plan
Added tests based on subset
type()tests.