-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: remove duplicate dropdown custom renderer #312
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
value: anyOfRenderInfo.label, | ||
})) | ||
const options = anyOfRenderInfos.map((anyOfRenderInfo) => { | ||
const option = String(anyOfRenderInfo.schema.const || anyOfRenderInfo.label) |
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.
why choose schema.const
over label
?
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.
The const resembles the actual value that the user will select in the case of a string anyOf, so it would be more appropriate to display that to the user. The label is only used for non-string anyOf (which will not have a const).
useEffect(() => { | ||
handleChange(path, schema.const) | ||
}, [handleChange, path, schema]) | ||
|
||
return null |
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 feels abit sus - what it feels like we're trying to do here is to write through to our underlying data structure (our data) but not render anything.
i think the pattern that we might want to aim for here might be to use a middleware + reducer so we fire the event only on update and only to mutate data
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.
Will likely remove this, I realise we won't have a case where the user will immediately enter with this FormBuilder without the type
being provided, so we are guaranteed to have this set.
Actually yeah removed in 492aa3f.
const [dropdownValue, setDropdownValue] = useState(data || "") | ||
|
||
if (!options || (options.length === 1 && !!schema.default)) { | ||
// Use the default value if it exists | ||
useEffect(() => { | ||
handleChange(path, schema.default || schema.const) | ||
}, [path, schema.default, schema.const, handleChange]) | ||
|
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 do we feel about setting the default value in teh useState
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 think the useState
not too important here cos the component is not rendered but I realised this also will cause changes even if the original data is defined. Fixed in 5717403.
492aa3f
to
594b2ac
Compare
...ures/editing-experience/components/form-builder/renderers/controls/JsonFormsAnyOfControl.tsx
Outdated
Show resolved
Hide resolved
...s/editing-experience/components/form-builder/renderers/controls/JsonFormsDropdownControl.tsx
Outdated
Show resolved
Hide resolved
594b2ac
to
bc18abe
Compare
993b9c9
to
80c3d74
Compare
0864776
to
c0fcd6b
Compare
Problem
A duplicate dropdown renderer appears due to the anyOf renderer and the dropdown custom renderer. This was due to the switch of the schema from being hand-written to TypeBox.
Solution
Breaking Changes
Bug Fixes:
anyOf-0
,anyOf-1
, etc).Before & After Screenshots
BEFORE:
AFTER: