-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[fields] Use onChange
instead of onKeyPress
for Backspace editing
#10494
[fields] Use onChange
instead of onKeyPress
for Backspace editing
#10494
Conversation
5ab2ab4
to
b020d02
Compare
@@ -213,6 +213,10 @@ export const useFieldState = < | |||
tempValueStrAndroid: null, | |||
})); | |||
|
|||
if (valueManager.areValuesEqual(utils, state.value, value)) { |
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.
Before, we were not even calling publishValue
.
But it's problematic with Backspace
because the state was not updated when pressing Backspace
on an already empty section, causing the selection to be removed by the browser.
So now we always update the state but we don't publish onChange
if not needed
const activeDateManager = fieldValueManager.getActiveDateManager(utils, state, activeSection); | ||
|
||
const nonEmptySectionCountBefore = activeDateManager | ||
.getSections(state.sections) | ||
.filter((section) => section.value !== '').length; | ||
const isTheOnlyNonEmptySection = nonEmptySectionCountBefore === 1; | ||
const hasNoOtherNonEmptySections = |
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.
Small fix when the active section is already empty
If could never happen before
Deploy preview: https://deploy-preview-10494--material-ui-x.netlify.app/ |
onChange
instead of onKeyPress
for Backspace editing
b020d02
to
a3778e4
Compare
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.
Great work! 💯
This has the added benefit of now allowing the full date clearing.
Previously clicking Backspace
on Android while having the whole field selected would not correctly clear the field 🙈
I noticed that after these changes there is some sort of flickering happening when deleting a section value on Android, it also happens when trying to delete already empty section.
It seems that visually the section value gets removed and then added back in.
Is there any way we could avoid this, because the UX is not the best now. 🙈
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
I I understand the flickering you are talking about, thenI also have the flickering on master mui-x/packages/x-date-pickers/src/internals/hooks/useField/useField.ts Lines 457 to 460 in 8b67c90
I do agree that it's not optimal |
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.
Sorry for missing the mentioned comment. 🙈
Yeah, it's definitely not a big problem, it's not that you constantly need to delete already empty sections. 😆
The mentionned comment is in the middle of thousands of lines of code, not easy to spot 👌 |
Fixes #10399
After some more investigation, the problem is that the
Backspace
key is always treated asUnidentified
on Android, so it was edited usingonChange
.I updated the
onChange
to work with Backspace editing, removed the Backspace editing fromonKeyPress
and updated all the tests.