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

Consider reverting "Make fields with const pre-fiiled and readonly" #4344

Closed
4 tasks done
MarekBodingerBA opened this issue Oct 23, 2024 · 7 comments · Fixed by #4381
Closed
4 tasks done

Consider reverting "Make fields with const pre-fiiled and readonly" #4344

MarekBodingerBA opened this issue Oct 23, 2024 · 7 comments · Fixed by #4381

Comments

@MarekBodingerBA
Copy link
Contributor

MarekBodingerBA commented Oct 23, 2024

Prerequisites

What theme are you using?

core

Version

5.22.0

Current Behavior

The recent implementation of prefilled, readonly fields for const values, introduced by #4326, has led to a regression impacting various use cases.

In my opinion, this change is a regression as it alters expected behaviors in forms where user interaction is required. There are legitimate scenarios where fields with const values should not be readonly, such as (see playground):

  • confirmation fields where users must manually enter a specific text,
  • agree to terms by interacting with a checkbox.

Attempting to override the prefilled value with default does not work as expected, further complicating form configuration. The implementation should not automatically control whether a field is readonly. Flexibility should be provided to developers to define this behavior based on the specific use case.

Expected Behavior

Developers should have the option to configure whether a const field is prefilled and readonly or requires user input.

Steps To Reproduce

  1. Open the playground.

Environment

No response

Anything else?

No response

@MarekBodingerBA MarekBodingerBA added bug needs triage Initial label given, to be assigned correct labels and assigned labels Oct 23, 2024
@heath-freenome heath-freenome added awaiting response and removed needs triage Initial label given, to be assigned correct labels and assigned labels Oct 25, 2024
@heath-freenome
Copy link
Member

@MarekBodingerBA According to the spec a const value for a field restricts that field to have one value (i.e. it can never be changed). Is the idea that const for optional fields should not prefill? Also, you could use the uiSchema to force the field to be readonly: false I believe. Did you want to add an experimental flag that doesn't prefill const values? Let's talk about this some more.

@nickgros
Copy link
Contributor

nickgros commented Oct 25, 2024

If you can prefill your form data and configure your uiSchema, you could do something like this to get the behavior you're looking for.

But if that's not an option for you, we might need to look into new feature flags.

@heath-freenome
Copy link
Member

Ah wait, we also made the field readonly if const was specified. Perhaps that is what you are asking to roll-back?

@MarekBodingerBA
Copy link
Contributor Author

@heath-freenome While I understand the spec's requirements, there might be legitimate use cases, such as an "I Agree" checkbox that should ultimately be checked true but not initially. How would you handle such a case?

Regarding "readonly": What's the standard for this library? Are other data types automatically set to readonly based on the schema, or is this the only case? If it's the only case, it should be considered whether this behavior is wanted for others or if there are other instances and it's consistent with the library's expected behavior.

@nickgros We unfortunately cannot prefill our form data due to the highly dynamic nature of our forms. We actually use getDefaultFormState on the backend to prefill the data before page rendering.

I think there are three options:

  • Revert this change (probably not desired by others)
  • Create a custom flag that won't prefill defaults for constants
  • Make it respect defaults ({const: true, default: false}), though I'm unsure if this complies with the spec

What are your thoughts?

Thanks for all your work on this.

@heath-freenome
Copy link
Member

heath-freenome commented Nov 8, 2024

@MarekBodingerBA Here's what we are proposing to do:

  1. We will remove the automatic readonly of a const. If you want that behavior, use the uiSchema
  2. We will be adding the following new Experimental_DefaultFormStateBehavior flag to support what you need:
 /** Optional enumerated flag controlling how const values are merged into the form data as defaults when dealing with
   * undefined values, defaulting to `always`. The defaulting behavior for this flag will always be controlled by the
   * `emptyObjectField` flag value. For instance, if `populateRequiredDefaults` is set and the const value is not
   * required, it will not be set.
   * - `always`: A const value will always be merged into the form as a default. If there is are const values in a
   *        `oneOf` (for instance to create an enumeration with title different from the values), the first const value
   *        will be defaulted.
   * - `skipOneOf`: If const is in a `oneOf` it will NOT pick the first value as a default
   * - `never`: A const value will never be used as a default.
   *
   */
  constAsDefaults?: 'always' | 'skipOneOf' | 'never';

Thoughts?

@MarekBodingerBA
Copy link
Contributor Author

@heath-freenome Perfect! Would it make sense to support {const: true, default: false} also, or is it something that doesn't really fit?

@heath-freenome
Copy link
Member

That would break the spec since const indicates that it is the only value a field can have

heath-freenome added a commit to heath-freenome/react-jsonschema-form that referenced this issue Nov 14, 2024
…ression

Fixes rjsf-team#4344, rjsf-team#4361 and rjsf-team#4377
- In `@rjsf/utils`:
  - Updated the `Experimental_DefaultFormStateBehavior` type to add new optional `constAsDefaults` prop with three choices
  - Updated `getDefaultFormState()` to respond to the new `constAsDefaults` feature to limit `const` as defaults based on the `never` or `skipOneOf` choices
  - Added tests for `getDefaultFormState()` to verify the new feature
- In `@rjsf/core`:
  - Updated `SchemaField` to remove making the field readonly when const
- In `playground`:
  - Updated `Header` to add support for `constAsDefaults`
- Updated the `CHANGELOG.md` accordingly
heath-freenome added a commit that referenced this issue Nov 15, 2024
…ression (#4381)

Fixes #4344, #4361 and #4377
- In `@rjsf/utils`:
  - Updated the `Experimental_DefaultFormStateBehavior` type to add new optional `constAsDefaults` prop with three choices
  - Updated `getDefaultFormState()` to respond to the new `constAsDefaults` feature to limit `const` as defaults based on the `never` or `skipOneOf` choices
  - Added tests for `getDefaultFormState()` to verify the new feature
- In `@rjsf/core`:
  - Updated `SchemaField` to remove making the field readonly when const
- In `playground`:
  - Updated `Header` to add support for `constAsDefaults`
- Updated the `CHANGELOG.md` accordingly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants