Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

pass formContext to SchemaField #1330

Merged
merged 1 commit into from
Jun 29, 2019
Merged

pass formContext to SchemaField #1330

merged 1 commit into from
Jun 29, 2019

Conversation

ealtunyay
Copy link
Contributor

@ealtunyay ealtunyay commented Jun 21, 2019

Reasons for making this change

  1. Documentation states, "Props passed to a custom SchemaField are the same as the ones passed to a custom field.", but formContext is not passed
  2. Use case: Make context aware modifications to errors in the ui. These are the options I considered:
    1. Modify my json-schema. Not ideal. These changes have side-effects on other business related things.
    2. Handle state myself and pass formData, but formData is only for form values.
    3. Use a custom field template. However, errors are already rendered by the time the template render is reached, and you cannot rerender them using the default error render function located in SchemaField.
    4. Use custom fields, but custom fields do not handle errors.
    5. Use a custom SchemaField. Close, but no formContext. If it was passed one could theoretically make error related changes and pass everything along to the default SchemaField component.
  3. Number of open issues seemed high (224). I thought this might be better :)

If this is related to existing tickets, include links to them as well.

  1. Not really related, but searching through issues before forking I found this issue that expressed a similar use case but for custom fields. Seems like it was answered.

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@ealtunyay
Copy link
Contributor Author

After some further development I realize I missed some things:

  1. My change was insufficient to propogate formContext fully through the tree. Further plumbing would be necessary.
  2. formContext is passed to custom SchemaFields through the registry object making my change redundant.

Perhaps the docs could be a little clearer? The "Custom SchemaField" section and "The registry Object" sections are close to each other, but the quote I referenced above ("Props passed to a custom SchemaField are the same as the ones passed to a custom field.") is still misleading. Up until this point I considered the registry object a place to register custom components. I didn't expect to find formContext in there, and so I didn't read it carefully enough.

Only after working through my usecase did I stumble across formContext inside the registry. Only in hindsight do I see that the section on the registry object is clear: "The registry is passed down the component tree, so you can access it from your custom field and SchemaField components.", and formContext is clearly on the list of properties above it.

I'll leave this open for now in case anyone cares to comment or suggest making a change in the documentation.

@epicfaace
Copy link
Member

It looks like we have a few things here:

  • formContext is passed down to all fields except for SchemaField
  • registry is passed down to all fields except for TitleField and DescriptionField

For consistency's sake, I think it would be best to pass formContext to SchemaField. In a later version where we can make breaking changes it might be best to just pass down registry to all fields, not formContext.

@epicfaace epicfaace merged commit a8319d7 into rjsf-team:master Jun 29, 2019
@ealtunyay ealtunyay deleted the add-formcontext-to-schemafield branch July 1, 2019 05:34
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants