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

fix(fieldset): support root conditionals for fieldsets #23

Merged
merged 31 commits into from
Jul 3, 2023

Conversation

sandrina-p
Copy link
Collaborator

@sandrina-p sandrina-p commented Jul 2, 2023

Requires #18. Please review that one first. Then come back there.

This PR ensures that root conditionals (if) that have effects (then) in fieldsets are applied correctly.

Do not mix the two concepts:

  • Scoped conditionals: Conditionals written inside a fieldset - that works fine.
  • Conditionals to fieldsets: Root conditionals that affect a fieldset - the bug fixed in this PR.

Steps to replicate

Use the following JSON Schema to replicate the reported bug in our JSF playground:

  1. Copy the schema below
  2. Set high working hours (eg 40)
  • Bug: All "perks.food" options are still visible. Expectation: The "no" should be hidden.
{
  "additionalProperties": false,
  "type": "object",
  "properties": {
    "work_hours_per_week": {
      "title": "Hours per week",
      "type": "number",
      "description": "Above 30 hours, the Perk>Food options change, and PTO is required.",
      "x-jsf-presentation": {
        "inputType": "number"
      }
    },
    "pto": {
      "title": "Time-off (days)",
      "type": "number",
      "x-jsf-presentation": {
        "inputType": "number"
      }
    },
    "perks": {
      "additionalProperties": false,
      "properties": {
        "food": {
          "oneOf": [
            {
              "const": "lunch",
              "title": "Lunch"
            },
            {
              "const": "dinner",
              "title": "Dinner"
            },
            {
              "const": "all",
              "title": "All",
              "description": "Every meal"
            },
            {
              "const": "no",
              "title": "No food"
            }
          ],
          "title": "Food",
          "type": "string",
          "x-jsf-presentation": {
            "inputType": "radio"
          }
        },
        "retirement": {
          "oneOf": [
            {
              "const": "basic",
              "title": "Basic"
            },
            {
              "const": "plus",
              "title": "Plus"
            }
          ],
          "title": "Retirement",
          "type": "string",
          "x-jsf-presentation": {
            "inputType": "radio"
          }
        }
      },
      "required": ["food", "retirement"],
      "title": "Perks",
      "type": "object",
      "x-jsf-presentation": {
        "inputType": "fieldset"
      }
    }
  },
  "allOf": [
    {
      "if": {
        "properties": {
          "work_hours_per_week": {
            "minimum": 30
          }
        },
        "required": ["work_hours_per_week"]
      },
      "then": {
        "properties": {
          "pto": {
            "$comment": "@BUG: This description does not disappear once activated.",
            "description": "Above 30 hours, the PTO needs to be at least 20 days.",
            "minimum": 20
          },
          "perks": {
            "properties": {
              "food": {
                "description": "Above 30 hours, the 'no' option disappears.",
                "oneOf": [
                  {
                    "const": "lunch",
                    "title": "Lunch"
                  },
                  {
                    "const": "dinner",
                    "title": "Dinner"
                  },
                  {
                    "const": "all",
                    "title": "all"
                  }
                ]
              }
            }
          }
        },
        "required": ["pto"]
      }
    }
  ],
  "required": ["perks", "work_hours_per_week"]
}

How to test this:

  • Read Linear issue (private)
  • Check the unit tests and code comments. My apologies for not having a Loom here.

Tip for Remoters: To quickly preview this branch in our Playground, go to useCreateHeadlessForm.js (our internal hook), and replace the first line:

// This assumes the JSF repo is in the same folder as our internal FE repo.
- import { createHeadlessForm } from '@remoteoss/json-schema-form';
+ import { createHeadlessForm } from '@/src/../../json-schema-form/src';

@sandrina-p sandrina-p self-assigned this Jul 2, 2023
@sandrina-p sandrina-p changed the base branch from main to rmt-23-jsf-select-validation-is-incomplete July 2, 2023 17:14
@sandrina-p sandrina-p force-pushed the rmt-517-support-root-conditionals-for-fieldsets branch from ee05936 to 1c7ed57 Compare July 2, 2023 17:48
src/calculateConditionalProperties.js Show resolved Hide resolved
src/calculateConditionalProperties.js Show resolved Hide resolved
src/helpers.js Show resolved Hide resolved
src/tests/createHeadlessForm.test.js Show resolved Hide resolved
src/tests/createHeadlessForm.test.js Show resolved Hide resolved
src/tests/createHeadlessForm.test.js Show resolved Hide resolved
@sandrina-p sandrina-p changed the title Draft: fix(fieldset): support root conditionals for fieldsets fix(fieldset): support root conditionals for fieldsets Jul 2, 2023
@sandrina-p sandrina-p force-pushed the rmt-23-jsf-select-validation-is-incomplete branch from 58e0efc to 93fbd2e Compare July 3, 2023 08:22
Copy link
Collaborator

@brennj brennj left a comment

Choose a reason for hiding this comment

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

One thing I'm struggling to understand going through the example

Screenshot 2023-07-03 at 13 05 37

Why are the required messages not showing up? Is this an issue just with our components? But we know radio fields definitely have required error messages when needed 🤔 I'm nearly certain i've seen them in fieldsets too.

Still looking through this

@posthardcode
Copy link

posthardcode commented Jul 3, 2023

One thing I'm struggling to understand going through the example

Screenshot 2023-07-03 at 13 05 37 Why are the required messages not showing up? Is this an issue just with our components? But we know radio fields definitely have required error messages when needed 🤔 I'm nearly certain i've seen them in fieldsets too.

Still looking through this

@brennj I have encountered this issue where the required message didn't show up, only the "invalid" one. I didn't look into the cause (since the actual issue was something else), but this thread might help.

src/tests/helpers.js Outdated Show resolved Hide resolved
src/tests/helpers.js Outdated Show resolved Hide resolved
@brennj
Copy link
Collaborator

brennj commented Jul 3, 2023

I think we'll be good to merge this one in itself, but pending we fix the other branch first

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "@remoteoss/json-schema-form",
"version": "0.4.0-beta.0",
"version": "0.4.1-dev.20230703082439",
Copy link
Collaborator

Choose a reason for hiding this comment

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

just need to change this and i think we're all good :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we'll be good to merge this one in itself, but pending we fix the other branch first

Cool! Let me wrap up the #18 first and then I'll come back to this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#18 merged, let me rebase this one, it will take a few minutes, as this is a branch of branch 👀

Copy link
Collaborator

Choose a reason for hiding this comment

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

on standby!

Copy link
Collaborator Author

@sandrina-p sandrina-p Jul 3, 2023

Choose a reason for hiding this comment

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

Ah, I can't rebase as Gabriel has used merged before, so I used merge too. Easy and quick. it's whatever as we'll squash it all afterward.

I've also reverted the -dev version with npm run version_as_main

@sandrina-p sandrina-p force-pushed the rmt-517-support-root-conditionals-for-fieldsets branch from 3b007cf to ec2b2eb Compare July 3, 2023 20:32
Base automatically changed from rmt-23-jsf-select-validation-is-incomplete to main July 3, 2023 22:02
Copy link
Collaborator

@brennj brennj left a comment

Choose a reason for hiding this comment

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

lets gooo

@sandrina-p sandrina-p merged commit 65d87b3 into main Jul 3, 2023
3 checks passed
@sandrina-p sandrina-p deleted the rmt-517-support-root-conditionals-for-fieldsets branch July 3, 2023 22:20
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.

4 participants