Mouse events have key events rule #849
Conversation
JoshuaKGoldberg
added
the
PR: Waiting for Author
JoshuaKGoldberg
added
PR: Waiting for Reviewer
| <div onMouseOver={() => {}} {...props} /> | ||
| <div onMouseOut={() => {}} {...props} /> | ||
| <div /> | ||
| <div {...props} /> |
There was a problem hiding this comment.
These four cases, where the line starts with <, are invalid TypeScript. You'll need to store these in a variable as with the other cases.
There was a problem hiding this comment.
Oh oops.
Ok, so
const foo = <div onMouseOver={() => {}} {...props} />
const bar = <div onMouseOut={() => {}} {...props} />
does complain, which makes sense. So do you just ignore elements with spread attributes?
There was a problem hiding this comment.
I think so, yeah. Without a program's type checker there's no way to know if they're valid.
JoshuaKGoldberg
added
PR: Waiting for Author
| const Foo = (<div onMouseOver={() => {}} {...props} />) | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [onMouseOver must be accompanied by onFocus.] | ||
| const Bar = (<div onMouseOut={() => {}} {...props} />) | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [onMouseOut must be accompanied by onBlur.] |
There was a problem hiding this comment.
@lizzzp1 could you change it to not complain if there's a ..., actually? It would be annoying to have a ...props that includes the correct corresponding onBlue/onFocus and still have this complain.
There was a problem hiding this comment.
I thought that required the type checker that was a follow up? I'm really confused now.
There was a problem hiding this comment.
The type checker is just required to know whether the ...props contains the corresponding onBlur/onFocus. Without the type checker, we have know way of knowing whether a JSX element that contains ...props violates the rule or not. Until the type checker logic is added, that leaves us with two options:
- Ignore
...props_(pretend they aren't therE): the rule would give complaints in some places where it shouldn't - Assume
...propscontains the right attributes: the rule wouldn't give complaints in some places where it should
In order to not irritate folks with false complaints, IMO we should err on the side of caution with the first option.
Does that seem reasonable?
There was a problem hiding this comment.
Ok, if props are ignored then it would complain like the above wouldn't it? Wouldn't the 2nd option be better so it doesn't complain if it encounters spread props?
There was a problem hiding this comment.
Oh agh sorry I meant to say the second option! You're absolutely correct 😅
| const Foo = (<div onMouseOver={() => {}} {...props} />) | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [onMouseOver must be accompanied by onFocus.] | ||
| const Bar = (<div onMouseOut={() => {}} {...props} />) | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [onMouseOut must be accompanied by onBlur.] |
There was a problem hiding this comment.
@lizzzp1 could you change it to not complain if there's a ..., actually? It would be annoying to have a ...props that includes the correct corresponding onBlue/onFocus and still have this complain.
There was a problem hiding this comment.
This looks superb, thanks for working through reviews on this @lizzzp1! 🙌
PR checklist
Overview of change:
#279
Adds rule for
Enforce that onMouseOver/onMouseOut are accompanied by onFocus/onBlur for keyboard-only users.Planning on adding the next few, but starting here to make sure this is heading in the right direction.
Is there anything you'd like reviewers to focus on?