Allow typeIn for contenteditable elements
#869
Conversation
| @@ -46,27 +46,30 @@ export default function typeIn(target: Target, text: string, options: Options = | |||
| throw new Error('Must pass an element or selector to `typeIn`.'); | |||
| } | |||
|
|
|||
| const element = getElement(target); | |||
| const element = getElement(target) as Element | HTMLElement; | |||
There was a problem hiding this comment.
Curious why we need to cast this
There was a problem hiding this comment.
Partially because that's what fillIn was doing. There was a type error that I don't recall. I'll go check on it.
There was a problem hiding this comment.
kk, doesn't seem like a major issue to me, but if we have a type misalignment in getElement we can try to fix it instead of forcing all consumers to cast. Alternatively, if specific helper methods can only work on HTMLElement (vs the more generic Element) maybe we should make an assertion method that narrows the types for us.
There was a problem hiding this comment.
I pushed a commit to this branch that alters the type guards and removes the casting but it's not reflecting in the PR 🤷
There was a problem hiding this comment.
One bit to note here about how both fillIn and typeIn work is they're assuming FormControl is ok, but that includes selects and buttons which don't seem like they should be allowed. Might be nice to have some sort of fillable type guard that narrows down to input/textarea and maybe content editable?
There was a problem hiding this comment.
Yeah, totally agree. Seems like a great candidate for a follow up PR if you have the time.
| __focus__(element); | ||
| } else if (isContentEditable(element)) { | ||
| __focus__(element); |
There was a problem hiding this comment.
I would generally prefer to avoid having these two branches that each call __focus__, think there is a way to unify a bit to avoid the extra branching?
There was a problem hiding this comment.
Yeah! That's definitely doable. Mostly just mirroring the structure of fillIn
There was a problem hiding this comment.
Yep, makes sense, thanks for working on it!
|
Thanks again @jeffhertzler! |
rwjblue
changed the title
typeIn for contenteditable elements
resolve #793