Skip to content
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

Use default values when switching anyOf option #4375

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

quentin-sommer
Copy link
Contributor

@quentin-sommer quentin-sommer commented Nov 11, 2024

Reasons for making this change

The UI wasn't properly handling the option change resulting in undefined data (See my video in #4367).

In summary this change makes this schema work as expected:

{
  "additionalProperties": false,
  "properties": {
    "any_of_array_or_null": {
      "anyOf": [
        {
          "items": {
            "type": "integer"
          },
          "type": "array",
          "title": "Array value selected",
          "default": []
        },
        {
          "type": "null",
          "title": "Null value selected"
        }
      ]
    }
  },
  "required": [
    "any_of_array_or_null"
  ],
  "type": "object"
}

But when removing the default value for the array field the UI doesn't render a + button. I have no idea how/where to fix this. In the playground an array without data is rendered with a +. I expect there is a function somewhere that can provide a default value for this

Works after @nickgros's suggestion

fixes #4367

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests. I've run npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update to update snapshots, if needed.
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@nickgros
Copy link
Contributor

nickgros commented Nov 15, 2024

@quentin-sommer A couple of thoughts:

Does your change fix the original issue in #4367 ? When I tried it locally it did not seem to fix it, but I want to confirm with you.

Instead of changing sanitizeDataForNewSchema, it seems like we could let getDefaultFormState handle this case. Since sanitizeDataForNewSchema is only called in MultiSchemaField, what do you think of changing MultiSchemaField L126 from

    if (newFormData && newOption) {

to

    if (newOption) {

Your test seems to pass with this change, but I want to make sure I'm not missing an edge case I am unaware of.

@quentin-sommer
Copy link
Contributor Author

quentin-sommer commented Nov 18, 2024

I applied your suggestion and it fixes the original issue in #4367 as well as the schema in this PR description, thanks!

I also added a changelog entry. Let me know if this looks good to go

heath-freenome
heath-freenome previously approved these changes Nov 22, 2024
packages/utils/src/schema/sanitizeDataForNewSchema.ts Outdated Show resolved Hide resolved
@@ -165,6 +165,43 @@ describe('anyOf', () => {
);
});

it('should assign a default value and set defaults on option change for scalar types schemas', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a similar test for the oneOf.test.jsx file?

Copy link
Contributor Author

@quentin-sommer quentin-sommer Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing, PR updated 👍

@heath-freenome heath-freenome dismissed their stale review November 22, 2024 17:21

Just noticed one more change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to switch between the options in the anyOf when omitExtraData and liveOmit is true
3 participants