Fix/datepicker validation by LukasBoll · Pull Request #2200 · eclipsesource/jsonforms

LukasBoll · 2023-10-27T11:35:49Z

Previously, data wasn't cleared when the input field was emptied.
This commit will set the value to 'undefined' when the input is empty or
invalid upon blurring. During editing, the data is only updated when the
current input is valid.

Closes #2183

netlify · 2023-10-27T11:35:53Z

✅ Deploy Preview for jsonforms-examples ready!

Name	Link
🔨 Latest commit	`6e5cffe`
🔍 Latest deploy log	https://app.netlify.com/sites/jsonforms-examples/deploys/659d81d9f1d18b0008205ff4
😎 Deploy Preview	https://deploy-preview-2200--jsonforms-examples.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

coveralls · 2023-10-31T00:24:50Z

coverage: 84.82% (+0.03%) from 84.795%
when pulling 6e5cffe on LukasBoll:fix/datepicker_validation
into c8d3ecf on eclipsesource:master.

sdirix · 2023-10-31T09:41:47Z

New behavior:

During editing, when value is a valid date/time, immediately write through
During editing, when value is not valid, don't trigger any change
On blur, if value is not valid, write undefined in data

sdirix

In principal looks good, however I found some issues in the code. Please also test the exceptional cases for which we require all that custom blur update handling.

sdirix · 2023-11-13T22:01:55Z

+  const updateChild = () => {
+    setKey(key + 1);
+  };


Whenever the new value of a state depends on the old value, the functional updater should be used, i.e. const updateChild = () => setKey((key) => key + 1)

sdirix · 2023-11-13T22:02:57Z

+  const onBlur = useMemo(
+    () =>
+      createOnBlurHandler(path, handleChange, format, saveFormat, updateChild),
+    [path, handleChange, format, saveFormat, updateChild]
+  );


This useMemo will never work as updateChild is a new instance on every render call, therefore always breaking the memo.

sdirix · 2023-11-13T22:03:35Z

    appliedUiSchemaOptions.showUnfocusedDescription
  );

+  const [key, setKey] = useState<any>(0);


We should avoid any at all cost and only use it when there is no better way. Here we can just type as number.

sdirix · 2023-11-13T22:03:52Z

  const format = appliedUiSchemaOptions.dateTimeFormat ?? 'YYYY-MM-DD HH:mm';
  const saveFormat = appliedUiSchemaOptions.dateTimeSaveFormat ?? undefined;

+  const [key, setKey] = useState<any>(0);


The key here is never used

sdirix · 2023-11-13T22:04:05Z

  const appliedUiSchemaOptions = merge({}, config, uischema.options);
  const isValid = errors.length === 0;

+  const [key, setKey] = useState<any>(0);


The key here is never used

sdirix · 2023-11-13T22:05:43Z


 export const MaterialDateControl = (props: ControlProps) => {
-  const [focused, onFocus, onBlur] = useFocus();
+  const [focused, onFocus] = useFocus();


We lose the focus handling of useFocus here, i.e. it expects that onBlur is called at some point to remove the focus. Shouldn't we call the onBlur here in our custom onBlur?

sdirix

one minor change request regarding the naming. besides that, look's good!

sdirix · 2023-11-20T12:27:36Z

+    format: string | undefined,
+    saveFormat: string | undefined,
+    rerenderChild: () => void,
+    resetFocus: () => void


This method does not care why this callback is handed over. The parameter should be called onBlur or callback

sdirix · 2023-11-20T12:28:39Z

    : null;
  const secondFormHelperText = showDescription && !isValid ? errors : null;

+  const updateChild = useCallback(() => setKey((key) => key + 1), [setKey]);


technically setKey is not required to be in the dependency list as it's guaranteed to be stable by React, however adding it doesn't break anything.

sdirix

There is still some error lurking. To reproduce:

Open the React Material Ui example application (link)
Into the birthDate field, enter the year 900. Observe that the data is not updated as intended because this year is invalid
Blur the field, e.g. by clicking into another one
Observe that the field is now empty, however the invalid date was saved into the data
Refocus the field and blur it again
Now the invalid date is actually removed from the data

Expected behavior:
After executing steps 1-3, the field and the actual data should be empty.

LukasBoll · 2023-12-20T12:02:04Z

Hi @sdirix ,
this commit will change the default format of datetime to "YYYY-MM-DDTHH:mm:ss.sssZ," making it coherent with convertDateToString wich is used for createDefaultValue in the core.
The previous default format was the default format from dayjs (current date in ISO8601, without fraction seconds, e.g. '2020-04-02T08:02:17-05:00')
Is this fine or should we keep the old default format?

sdirix · 2023-12-20T12:39:45Z

Is this fine or should we keep the old default format?

This is fine as sss is also valid ISO8601

sdirix · 2024-01-03T16:37:58Z

  const format = appliedUiSchemaOptions.timeFormat ?? 'HH:mm';
-  const saveFormat = appliedUiSchemaOptions.timeSaveFormat ?? 'HH:mm:ss';
+  const saveFormat = appliedUiSchemaOptions.timeSaveFormat ?? defaultTimeFormat;


If a time is missing the seconds, the UI now doesn't show it anymore. See the dates example. Is there a way to still show it?

The previous implementation didn’t support formats without seconds by default. So this wouldn't be an unexpected/new behavior.
Currently I adjusted the examples in our application to use the HH:mm:ss format.
But it is also possible to check if the time-value uses the HH:mm:ss or HH:mm format and set the saveFormat accordingly. Would you prefer this solution?

sdirix · 2024-01-03T16:39:20Z

+  // Workaround to address a bug in Dayjs (https://github.com/iamkun/dayjs/issues/1849)
+  if (date.year() < 1000 && date.year() > 100) {


Please add tests for this. Also we should handle the range from `[0-100].

dayjs currently doesn't support years < 100, but I adjusted the code (+ tests) so these cases are also handled, in case dayjs will support them in the future.

Previously, data wasn't cleared when the input field was emptied. This commit will set the value to 'undefined' when the input is empty or invalid upon blurring. During editing, the data is only updated when the current input is valid. Closes eclipsesource#2183

sdirix

LGTM! Thanks

LukasBoll force-pushed the fix/datepicker_validation branch from 85d995e to 3b41788 Compare October 31, 2023 00:13

LukasBoll changed the title ~~Fix/datepicker validation~~ WIP: Fix/datepicker validation Oct 31, 2023

LukasBoll force-pushed the fix/datepicker_validation branch from 3b41788 to f59dec5 Compare November 6, 2023 18:22

LukasBoll changed the title ~~WIP: Fix/datepicker validation~~ Fix/datepicker validation Nov 6, 2023

LukasBoll changed the title ~~Fix/datepicker validation~~ WIP: Fix/datepicker validation Nov 7, 2023

LukasBoll changed the title ~~WIP: Fix/datepicker validation~~ Fix/datepicker validation Nov 7, 2023

sdirix requested changes Nov 13, 2023

View reviewed changes

LukasBoll force-pushed the fix/datepicker_validation branch from f59dec5 to 74de9ae Compare November 15, 2023 14:07

sdirix reviewed Nov 20, 2023

View reviewed changes

LukasBoll force-pushed the fix/datepicker_validation branch from b1b36c8 to 53b5a73 Compare December 4, 2023 14:36

sdirix requested changes Dec 13, 2023

View reviewed changes

sdirix force-pushed the master branch 2 times, most recently from 8bd14c2 to e9946ef Compare December 15, 2023 16:07

LukasBoll force-pushed the fix/datepicker_validation branch 3 times, most recently from 67e6bdb to 81fece1 Compare December 20, 2023 11:24

sdirix reviewed Jan 9, 2024

View reviewed changes

LukasBoll force-pushed the fix/datepicker_validation branch from 81fece1 to 6e5cffe Compare January 9, 2024 17:26

sdirix approved these changes Jan 22, 2024

View reviewed changes

sdirix merged commit 01b08f0 into eclipsesource:master Jan 22, 2024

		// Workaround to address a bug in Dayjs (https://github.com/iamkun/dayjs/issues/1849)
		if (date.year() < 1000 && date.year() > 100) {

Uh oh!

Conversation

LukasBoll commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify Bot commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for jsonforms-examples ready!

Uh oh!

coveralls commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdirix commented Oct 31, 2023

Uh oh!

sdirix left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdirix left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdirix left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukasBoll commented Dec 20, 2023

Uh oh!

sdirix commented Dec 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdirix left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LukasBoll commented Oct 27, 2023 •

edited

Loading

netlify Bot commented Oct 27, 2023 •

edited

Loading

coveralls commented Oct 31, 2023 •

edited

Loading

sdirix left a comment •

edited

Loading