-
-
Notifications
You must be signed in to change notification settings - Fork 255
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
Allow typeIn
for contenteditable
elements
#869
Allow typeIn
for contenteditable
elements
#869
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.
Thanks for picking this up!
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why we need to cast this
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! That's definitely doable. Mostly just mirroring the structure of fillIn
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.
Yep, makes sense, thanks for working on it!
Thanks again @jeffhertzler! |
typeIn
for contenteditable
elements
resolve #793