[Select] Improve response, react to mouse down #17978
Conversation
Details of bundle changes.Comparing: 197b69c...5018f21
|
|
I added some tests (SarthakC#1) how I think this new behavior should work. |
Thank you. I will work on this and revert. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 0280b6b:
|
SarthakC
force-pushed
the
sarthak/simple-select
branch
from
678876a to
452bc99
Compare
It worked. I added a few more tests based on yours. |
eps1lon
added
bug 🐛
There was a problem hiding this comment.
@SarthakC Thanks for fixing the tests. I have looked at the behavior of the select, I have seen two regressions:
- https://deploy-preview-17978--material-ui.netlify.com/components/selects/#with-a-dialog. Open the dialog. Click to open and close the select. The focused state isn't dismissed.
- https://deploy-preview-17978--material-ui.netlify.com/components/selects/#simple-select. Click to open the select. Press arrow down. Nothing happens.
|
@SarthakC Hi. Do you think that you will be able to handle the regressions? |
@oliviertassinari Hello! Sorry for AWOL. I have been down with the flu. I shall be able to get back to this as soon as I get better. |
|
@SarthakC No problem, I hope you feel better soon :) |
Add tests for the new behavior changed Select.test.js
This reverts commit 1378ac4.
oliviertassinari
force-pushed
the
sarthak/simple-select
branch
from
777fa1a to
85458a2
Compare
oliviertassinari
force-pushed
the
sarthak/simple-select
branch
from
85458a2 to
58f66b9
Compare
|
@SarthakC I have pushed a two-line fixe and rebased with an update of the test. I hope it will do it and that you are doing better. |
oliviertassinari
changed the title
Thank you @oliviertassinari |
|
@SarthakC Thank you |
|
Wanted to mention we had similar breakage happen due to the Select onClick change from this PR, inside the <Select
renderValue={() => (
<TextField
// this no longer fires
onClick={e => doSomething(e)}
/>
)}
/>the solution is to now use |
|
@herrethan What's your use case for rendering a TextField here, with on click? @eps1lon What do you think of reverting these changes? We have seen a couple of negative effects from this change:
Does it worth it? And if it does, released under a patch, is this right? 🤔 |
|
@oliviertassinari thanks! We have a unique input field that is a combo between text input and select dropdown, where the select options are predefined values meant to be convenience selections that populate the text input.
On the TextField onClick we |
|
Right, the counter-arguments to revert are that 1. the patch was meant to improve user experience 2. the test regression impact unit tests (not e2e), tests that could be less dependent on the event but only consider the user actions. 3. People rendering custom interactive elements in the select, likely want to use https://material-ui.com/components/autocomplete/ instead. |
This would be bad precedent. It was a bug. People relying on this bug (especially in a test) should not block the fix.
We have to stop at some point. Customization doesn't mean you should do everything that's possible. Especially the example from @herrethan is a classic combobox. It was never intended to use the Select in that way. This is why it is important in open source to give back. Have a use case that isn't explicitly documented? Contribute a test case or documentation or open an issue if the use case is not trivial and let maintainers handle the integration. Not sure if it's trivial or not? Open anyway. I also want to emphasize that we have to be more confident in our judgement. We can't consider a revert because of a single issue report. Instead figure out how to best move forward. |
I explicitly addressed this one. It was relying on a bug. This should never block a fix. The special use case should be considered though. |
|
OK thanks. I think that we should leave it more time, to see how things settle. My main concern now (releasing it under a patch, that's too later) is with how people write unit tests, we are making it a bit harder for them but that's DX based and not UX based. |
No we are fixing a bug. You are now making an argument to never release fixes because people might rely on them in their software. |
|
I use the Select component inside a DataGrid cell. This change broke the logic to enter editing mode in the cell. Attempting to fix it breaks other parts of the DataGrid logic. I will have to spend a few hours getting everything working again. Given that the difference is perceived performance (between click and mouse down) is not even noticeable (at least to me), I wonder if this change was worth it. |
resolves #8999