[Select] Right click opens select menu #19434
Conversation
Details of bundle changes.Comparing: 5c84559...240573b
|
oliviertassinari
added
bug 🐛
oliviertassinari
added
the
PR: needs revision
|
@eps1lon , please take a look. I've moved the test to UnitTests |
eps1lon
added
PR: ready to ship
and removed
PR: needs revision
| @@ -105,6 +105,9 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) { | |||
| }; | |||
|
|
|||
| const handleMouseDown = event => { | |||
| if (event.button !== 0) | |||
There was a problem hiding this comment.
I'm surprised the CI passes without the bracket. Should we care or ignore it?
There was a problem hiding this comment.
To expand a bit: The point of a common formatting and linting ruleset is to not care about nitpicks in human code reviews. These discussions always slow down development. They can happen but on a per-ruleset basis not for each and every individual case.
Should we care about single-line brackets: Probably (main argument being diff noise here). But then we should change the rules globally.
There was a problem hiding this comment.
@eps1lon I couldn't agree more with you
There was a problem hiding this comment.
I had this rule in mind: https://github.com/airbnb/javascript#blocks--braces. If we agree to enforce it globally (I thought we were), I will add it to a backlog of small changes. It's not really important, I think that it could be handled later, like a Friday later afternoon, to rest.
|
Well done! |
|
Refactoring all of our unit tests to play nicely with this is really specific to this library, is there a simpler way to trigger the Select with enzyme? // even before
SelectField.find('[role="button"]').simulate('click');
// before
SelectField.find('[role="button"]').simulate('mousedown');
// now
SelectField.find('[role="button"]').simulate('mousedown', { button: 0 }); |
|
@Floriferous it's pretty much why we are moving away from enzyme. |
|
Don't get me wrong, I'd like to switch too, but that's a hard one to prioritize when you have to rewrite all of your tests :) Alright, we'll keep it like that. It does give us one more reason to push it up the priority list ! |
|
Agree, it's a hard tradeoff to make! Maybe you could change your test to simulate the onChange prop call, bypassing the interactions? |
I would suggest a different approach here. Add + import { fireEvent } from '@testing-library/react';
-SelectField.find('[role="button"]').simulate('mousedown');
+fireEvent.mouseDown(SelectField.find('[role="button"]').instance())This basic case seems like a good candidate for a codemod. The problem with enzyme is that it does not dispatch a proper event.
|
Close #19250