[Select] Right click opens select menu#19434
[Select] Right click opens select menu#19434oliviertassinari merged 7 commits intomui:masterfrom fyodor-e:select-opened-by-right-mouse-click
Conversation
Details of bundle changes.Comparing: 5c84559...240573b
|
eps1lon
left a comment
There was a problem hiding this comment.
Thanks for the fix.
However, it is a fundamental feature of the component and not just something that is interesting when integrating with a Dialog. I would rather see this in the corresponding unit test.
|
@eps1lon , please take a look. I've moved the test to UnitTests |
| }; | ||
|
|
||
| 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.
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