Use index for value in SelectWidget #1562

tdamsma · 2019-12-30T14:16:43Z

This PR changes the way options are selected in the SelectWidget by using the options array index instead of a serializing the selected value to the DOM (cast to string). This allows removal of a lot of hacky code to guess type of deserialized values, and also allows for selecting complete objects via a dropdown, addressing #1494.

I've added a playground example. I haven't looked at fixing the tests yet as would like feedback on the approach first.

epicfaace

This would be a great improvement!

epicfaace · 2020-01-11T19:51:04Z

packages/core/src/components/widgets/SelectWidget.js

+      return i;
+    }
+  }
+  return "";


Why return an empty string by default?

Could we just return undefined instead -- so that it would also be possible to select ""? (an additional improvement from the old code)

sounds like a good idea, will look into it

epicfaace · 2020-01-11T19:57:00Z

packages/core/src/components/widgets/SelectWidget.js

      }}>
      {!multiple && schema.default === undefined && (
        <option value="">{placeholder}</option>
      )}
      {enumOptions.map(({ value, label }, i) => {
        const disabled = enumDisabled && enumDisabled.indexOf(value) != -1;
        return (
-          <option key={i} value={value} disabled={disabled}>


This is likely a breaking change because it changes what's rendered in the DOM (the value)... but it's fine to go into version 2, and worth it to allow for better selection of any kinds of enums.

tdamsma · 2020-01-12T21:17:50Z

This would be a great improvement!

Good to hear, what can I do help this get landed? Not really sure how to proceed. Lots of tests are failing, but as they are written now they would become kind of pointless as most tests I saw focus on checking the values in the DOM render which are just a bunch of integers with this change.

epicfaace · 2020-01-12T22:48:52Z

It seems that many of the tests are failing because o is undefined in .map(o => o.value): https://github.com/rjsf-team/react-jsonschema-form/blob/master/packages/core/src/components/widgets/SelectWidget.js#L43

Fixes rjsf-team#1494, reimplementing rjsf-team#1562 to make all widgets that render `enumOptions` switch from option value to option index - In `@rjsf/utils`, added/updated `enumOptionsXXXX` methods that enable using index instead of values - Added 100% unit tests for these new methods - Also remove the now unnecessary `processSelectValue()` function and its unit test - In all themes, updated the `CheckboxesWidget`, `RadioWidget` and `SelectWidget` to use the `enumOptions[]` index rather than value for the html elements - Utilized the new `enumOptionsXXXX` methods to facilitate this transformation - Updated the documentation for the new methods and the breaking change this causes - Updated the `CHANGELOG.md` file accordingly

Fixes rjsf-team#1494, reimplementing rjsf-team#1562 to make all widgets that render `enumOptions` switch from option value to option index - In `@rjsf/utils`, added/updated `enumOptionsXXXX` methods that enable using index instead of values - Added 100% unit tests for these new methods - Also remove the now unnecessary `processSelectValue()` function and its unit test - In all themes, updated the `CheckboxesWidget`, `RadioWidget` and `SelectWidget` to use the `enumOptions[]` index rather than value for the html elements - Utilized the new `enumOptionsXXXX` methods to facilitate this transformation - Updated the tests in `core` and snapshots in the other themes to represent this change from value to index - Updated the documentation for the new methods and the breaking change this causes - Updated the `CHANGELOG.md` file accordingly

Fixes rjsf-team#1494, reimplementing rjsf-team#1562 to make all widgets that render `enumOptions` switch from option value to option index - In `@rjsf/utils`, added/updated `enumOptionsXXXX` methods that enable using index instead of values - Added 100% unit tests for these new methods - Also remove the now unnecessary `processSelectValue()` function and its unit test - In all themes, updated the `CheckboxesWidget`, `RadioWidget` and `SelectWidget` to use the `enumOptions[]` index rather than value for the html elements - Utilized the new `enumOptionsXXXX` methods to facilitate this transformation - Updated the tests in `core` and snapshots in the other themes to represent this change from value to index - Updated the `playground` to add the new `Enumerated Object` example - Updated the documentation for the new methods and the breaking change this causes - Updated the `CHANGELOG.md` file accordingly

Fixes rjsf-team#1494, reimplementing rjsf-team#1562 to make all widgets that render `enumOptions` switch from option value to option index - In `@rjsf/utils`, added/updated `enumOptionsXXXX` methods that enable using index instead of values - Added 100% unit tests for these new methods - Updated `optionId()` to take an index rather than an option - Also remove the now unnecessary `processSelectValue()` function and its unit test - In all themes, updated the `CheckboxesWidget`, `RadioWidget` and `SelectWidget` to use the `enumOptions[]` index rather than value for the html elements - Utilized the new `enumOptionsXXXX` methods to facilitate this transformation - Also passed index to `optionId()` rather than option - Updated the tests in `core` and snapshots in the other themes to represent this change from value to index - Updated the `playground` to add the new `Enumerated Object` example - Updated the documentation for the new methods and the breaking change this causes - Updated the `CHANGELOG.md` file accordingly

heath-freenome · 2023-01-30T08:08:16Z

Implemented this fully for all widgets that use enumOptions (not just SelectWidget) in #3411

* Fix: switch enumOptions rendering widgets to use indexes Fixes #1494, reimplementing #1562 to make all widgets that render `enumOptions` switch from option value to option index - In `@rjsf/utils`, added/updated `enumOptionsXXXX` methods that enable using index instead of values - Added 100% unit tests for these new methods - Updated `optionId()` to take an index rather than an option - Also remove the now unnecessary `processSelectValue()` function and its unit test - In all themes, updated the `CheckboxesWidget`, `RadioWidget` and `SelectWidget` to use the `enumOptions[]` index rather than value for the html elements - Utilized the new `enumOptionsXXXX` methods to facilitate this transformation - Also passed index to `optionId()` rather than option - Updated the tests in `core` and snapshots in the other themes to represent this change from value to index - Updated the `playground` to add the new `Enumerated Object` example - Updated the documentation for the new methods and the breaking change this causes - Updated the `CHANGELOG.md` file accordingly * Apply suggestions from code review Co-authored-by: Nick Grosenbacher <[email protected]> --------- Co-authored-by: Nick Grosenbacher <[email protected]>

epicfaace reviewed Jan 11, 2020

View reviewed changes

tdamsma force-pushed the select-by-index branch from 6e66412 to d3ccbd3 Compare July 9, 2020 07:10

tdamsma force-pushed the select-by-index branch from d3ccbd3 to afe1971 Compare September 2, 2020 15:01

tdamsma added 3 commits February 10, 2021 11:47

use index in select inputs instead of serialised value

0d00afb

change find function based on type of value

bc5b834

refactor the way selected items are determined

47aed5f

tdamsma force-pushed the select-by-index branch from afe1971 to 47aed5f Compare February 10, 2021 10:48

heath-freenome added the p1 Important to fix soon label Jan 15, 2023

heath-freenome mentioned this pull request Jan 30, 2023

Fix: switch enumOptions rendering widgets to use indexes #3411

Merged

8 tasks

heath-freenome closed this Jan 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use index for value in SelectWidget #1562

Use index for value in SelectWidget #1562

tdamsma commented Dec 30, 2019 •

edited

Loading

epicfaace left a comment

epicfaace Jan 11, 2020

tdamsma Jan 12, 2020

epicfaace Jan 11, 2020

tdamsma commented Jan 12, 2020

epicfaace commented Jan 12, 2020

heath-freenome commented Jan 30, 2023

Use index for value in SelectWidget #1562

Use index for value in SelectWidget #1562

Conversation

tdamsma commented Dec 30, 2019 • edited Loading

epicfaace left a comment

Choose a reason for hiding this comment

epicfaace Jan 11, 2020

Choose a reason for hiding this comment

tdamsma Jan 12, 2020

Choose a reason for hiding this comment

epicfaace Jan 11, 2020

Choose a reason for hiding this comment

tdamsma commented Jan 12, 2020

epicfaace commented Jan 12, 2020

heath-freenome commented Jan 30, 2023

tdamsma commented Dec 30, 2019 •

edited

Loading