-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat(core): add support for <optgroup>
tag
#4374
base: main
Are you sure you want to change the base?
Conversation
@nagaozen Thanks for the new feature... Can you update the |
Sure, must update the CHANGELOG and place some tests. |
Alright, checklist completed! |
@nagaozen Consider doing a |
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 still do not see any tests to verify that these changes work properly. You can update uiSchema.test.jsx
to add them there
const disabled = enumDisabled && enumDisabled.indexOf(value) !== -1; | ||
return ( | ||
<option key={i} value={String(i)} disabled={disabled}> | ||
{label} | ||
</option> | ||
); |
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 duplicate code to what is above. Consider creating a new component at the top of the file to render it and use it here and above
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.
Something like a helper component in the file:
interface OptionProps {
enumDisabled?: UIOptionsType['enumDisabled'];
value: EnumOptionsType['value'];
label: EnumOptionsType['label'];
index: number;
}
function Option(props: OptionProps) {
const { enumDisabled, index, label, value } = props;
const disabled = enumDisabled && enumDisabled.indexOf(value) !== -1;
return (
<option value={String(index)} disabled={disabled}>
{label}
</option>
);
}
Then this would look like:
Array.isArray(enumOptions) && enumOptions.map(({ value, label }, i) => {
return <Option key={i} enumDisabled={enumDisabled} index={i} value={value} label={label} />;
const disabled = enumDisabled && enumDisabled.indexOf(value) !== -1; | ||
let [{ label }, i] = enumOptionFromValue.get(value) | ||
return ( | ||
<option key={i} value={String(i)} disabled={disabled}> | ||
{label} | ||
</option> | ||
); |
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.
Duplicate code here, minus the let
line, which really should be const since you don't change anything
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.
Using the suggestion below, this would look like:
if (!enumOptionFromValue.has(value)) {
return null;
}
const [{ label }, i] = enumOptionFromValue.get(value);
return <Option key={i} enumDisabled={enumDisabled} index={i} value={value} label={label} />;
@nagaozen Also, since the |
Mmm, its a core widget, shouldn't it work as is in bootstrap 4? I've never played with the bs4 version, but will take a look into it. |
Co-authored-by: Heath C <51679588+heath-freenome@users.noreply.github.com>
@@ -16,6 +16,16 @@ should change the heading of the (upcoming) version to include a major version b | |||
|
|||
--> | |||
|
|||
# 5.23.0 |
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 will now be 5.24.0
since we just released 5.23.0
Reasons for making this change
HTML Select Element offers a native way to organize options in option groups. This update extends rjsf SelectWidget to support
<optgroup>
as well.If this is related to existing tickets, include links to them as well. Use the syntax
fixes #[issue number]
(ex:fixes #123
).fixes #1813, #580
If your PR is non-trivial and you'd like to schedule a synchronous review, please add it to the weekly meeting agenda: https://docs.google.com/document/d/12PjTvv21k6LIky6bNQVnsplMLLnmEuypTLQF8a-8Wss/edit
Checklist
npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update
to update snapshots, if needed.