-
Notifications
You must be signed in to change notification settings - Fork 413
fix how we handle empty values #2481
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
base: master
Are you sure you want to change the base?
fix how we handle empty values #2481
Conversation
…tionalProperties case)
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
@sdirix please review - when we update the handling of empty values it looks like deleting (not using the x - clear ) was setting the value to empty string rather to remove the property when we are not under dynamic context (e.g. additionalProperties where we need to preserve the key) |
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.
Changes make sense to me in general. However there are a number of "non-changes" in the PR. Also I just noticed that we might have an overall enum issue.
const clearValue = determineClearValue(''); | ||
return useVuetifyControl(useJsonFormsEnumControl(props), (value) => | ||
value === null ? clearValue : value, | ||
value !== null ? value : clearValue, |
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.
This is not an actual change.
return useVuetifyControl( | ||
useJsonFormsEnumControl(props), | ||
(value) => (value === null ? clearValue : value), | ||
(value) => (value !== null ? value : clearValue), |
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.
Thinking about it, this behavior is a bit dangerous for all enum related controls. null
could be a valid enum value. Why are we not using undefined
as the marker for the clearValue
again?
return useVuetifyControl( | ||
useJsonFormsOneOfEnumControl(props), | ||
(value) => (value === null ? clearValue : value), | ||
(value) => (value !== null ? value : clearValue), |
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.
Not a change
return useVuetifyControl( | ||
useJsonFormsEnumControl(props), | ||
(value) => (value === null ? clearValue : value), | ||
(value) => (value !== null ? value : clearValue), |
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.
Not a change
fix how we handle empty values when not in dynamic context (e.g. addtionalProperties case)