Pass through original values for invalid dates #1949
Conversation
|
|
|
Thank you very much. We're gonna take a look very soon ❤️ |
sdirix
left a comment
There was a problem hiding this comment.
The PR improves the situation but there are still problems. It seems that dayjs is very very lenient in regards of format and only reports Invalid Date when it can't match at all.
For example aaabbb1sad3asdasd00 in the time picker is parsed to 01:03:00 which might be unexpected for the user.
I would like to suggest to add an onBlur handler to the date, time and date-time renderers which makes sure that once the user leaves the input, the text-input is reset to reflect the actual data.
Do you agree? Can you add this?
Thank you for reviewing. I noticed this issue too and wasn't sure how to tackle it, but I like the "set value to parsed data on blur" idea, will add this in. |
|
@sdirix I got a chance to try and implement the "sync input values with parsed data on blur" improvement. However, it doesn't seem quite right especially with the failing unit tests (some of the errors are expected, but others seem to provide very different actual values from the expected values). When you get a chance, do you mind taking a look at this? |
sdirix
left a comment
There was a problem hiding this comment.
There are some issues with the current approach as for example the pickers no longer update the text input.
packages/material/src/util/datejs.ts
Outdated
| data: any; | ||
| onBlur: FormEventHandler<HTMLInputElement | HTMLTextAreaElement> | undefined; | ||
| }) => { | ||
| const [value, setValue] = useState(props.data); |
There was a problem hiding this comment.
This does not take into account cases where the data is changed differently, for example it will not update when data is changed via the pickers themselves or when the user hands in new data when externally updated.
packages/material/src/util/datejs.ts
Outdated
| const onBlur = useMemo( | ||
| () => (event: DateInputFormEvent) => { | ||
| setValue(props.data); | ||
| if (props.onBlur) props.onBlur(event); | ||
| }, | ||
| [props.data, props.onBlur] | ||
| ); |
There was a problem hiding this comment.
We can use onBlur directly however it's already used for focused. So instead of injecting ourselves into onBlur we can just check the value of focused.
packages/material/src/util/datejs.ts
Outdated
| const createOnChangeHandler = ( | ||
| onChange: (event: DateInputFormEvent) => void | ||
| ) => (event: DateInputFormEvent) => { | ||
| setValue((event.target as HTMLInputElement | HTMLTextAreaElement).value); | ||
| if (onChange) onChange(event); | ||
| }; |
There was a problem hiding this comment.
This way we completely bypass the current change semantics. It can certainly work but we need to keep all edge cases in mind. It's probably easier to "simply" transform the value handed in by the picker by ignoring it when we're not focused. This also makes it overall simpler as we don't manage another explicit state.
|
@sunnysingh I had a look and fixed the issues I found during review. Please test on your side whether it behaves the way you want to see. |
|
Hi, |
|
@sunnysingh I merged this now in. Please let me know if there is something which could be improved from your point of view. |
|
Thank you for fixing and merging @sdirix. The Also my apologies for not looking sooner, I had a few other things come up last week. |
This is a follow-up to the following community discussion: Showing errors for badly formatted date and time fields.
The current behavior attempts to format date input values with the dayjs library, and if the formatted string returns
Invalid Datethen it will callhandleChangewith anundefinedvalue. This not only causes dates to not show up in the finaldataobject, but it also makes it impossible to show validation errors for optional fields.This PR introduces a bugfix to pass through the original
keyboardInputValue, which triggers validation:This is my first contribution so please let me know if I'm missing anything.