-
Notifications
You must be signed in to change notification settings - Fork 832
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
Improve input mask UX and a11y #1661
Conversation
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/mui-org/material-ui-pickers/22ekls0i1 |
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.
What's the implication with rifm
, does it mean we no longer need it?
@@ -83,7 +80,7 @@ export const KeyboardDateInput: React.FC<DateInputProps & DateInputRefs> = ({ | |||
if ((rawValue === null || utils.isValid(rawValue)) && !isFocused) { | |||
setInnerInputValue(getInputValue()); | |||
} | |||
}, [rawValue]); // eslint-disable-line | |||
}, [rawValue, utils, inputFormat]); // eslint-disable-line |
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.
}, [rawValue, utils, inputFormat]); // eslint-disable-line | |
}, [rawValue, utils, inputFormat]); |
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.
There is a side-effect vakye isFocused
which should not be used as a dependency. Do you think we need to move it to refs?
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.
I don't follow. If we use isFocused
in the logic, why not include it in the dependency? If it doesn't have a negative side effect, and if it allows us to removes eslint-disable-line
. Then, it could help us catch issues after a future refactoring?
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.
We don't need to run effect if the input is changed state from focus to blur. We only need to handle it differently by the fact of focused state.
So this focused state does not need to be state actually. It can be a ref
const isFocusedRef = React.useRef(false)
onFocus={() => isFocusedRef.current = true}
onBlur={() => isFocusedRef.current = false}
And I am actually thinking about making it a ref...
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.
+1 for using a ref if we don't need to rerender.
I can see emptyInputText
as a dependency that is also ignored. Is this a dead prop?
@@ -44,7 +44,7 @@ export function usePickerState<TInput, TDateValue>( | |||
if (!valueManager.areValuesEqual(pickerDate, date)) { | |||
setPickerDate(date); | |||
} | |||
}, [value]); // eslint-disable-line | |||
}, [value, utils]); // eslint-disable-line |
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.
}, [value, utils]); // eslint-disable-line | |
}, [value, utils]); |
The logic of masked input is still working through the rifm, it is actually a tool for this (saving cursor position during input formats). |
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
Codecov Report
@@ Coverage Diff @@
## next #1661 +/- ##
==========================================
- Coverage 89.76% 89.26% -0.50%
==========================================
Files 73 73
Lines 2315 2320 +5
Branches 402 423 +21
==========================================
- Hits 2078 2071 -7
- Misses 190 198 +8
- Partials 47 51 +4
Continue to review full report at Codecov.
|
@@ -81,15 +86,23 @@ export const DateRangePickerInput: React.FC<DateRangeInputProps> = ({ | |||
} | |||
}, [currentlySelectingRangeEnd, open]); | |||
|
|||
// TODO: rethink this approach. We do not need to wait for calendar to be updated to rerender input (looks like freezing) | |||
// TODO: so simply break 1 react's commit phase in 2 (first for input and second for calendars) by executing onChange in the next tick | |||
const lazyHandleChangeCallback = React.useCallback( |
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.
Described more in this twitter thread
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.
Is the underlying issue that the calendar (containing ~30 days instances) too slow to update?
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.
Yeah it is slow, cause actually 70+ days for standard desktop calendar.
Moreover it is working like throttle but on the react level. User doesn’t expect to see January of 0002 year when input is 01/01/2
Linked issues:
keepCharPositions
parameter in version 3.1.0 #1144Breaking changes
maskChar
prop, because it is not visible for users anymore and need only to generate and parse mask pattern.Features
Change the logic of the input mask to be more accessible and less noisy.