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

Move label gen from TexWidget to FieldTemplate for bootstrap4 #2533

Closed
wants to merge 1 commit into from

Conversation

newt10
Copy link
Collaborator

@newt10 newt10 commented Sep 2, 2021

Reasons for making this change

  1. The current code for FieldTemplate and TextWidget in bootstrap 4 is incompatible with the core package. This breaks the
    external facing API for customizing field template.
  2. According to the core package implementation and documents, FieldTemplate is responsible for layout of labels, description, errors and help while the input widget along with the state is to be managed by the Field Component themselves (and Widgets)
  3. If a user, uses custom field template with the bootstrap 4 form, the label is displayed multiple times, once by the custom field template (as intended in the API) and second time by the TextWidget (breaks from the core package convention)

This fixes the problem and updates the tests to catch any future regressions.

If this is related to existing tickets, include links to them as well. Use the syntax fixes #2007 (ex: fixes #123).

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

Since this is a bug fix, no docs need to be updated, docs already provide the correct information on how to use custom field templates.

I have updated all the tests

Test from Playground

Screen Shot 2021-09-01 at 2 14 25 PM

All tests passing

Screen Shot 2021-09-01 at 6 20 44 PM
Screen Shot 2021-09-01 at 6 22 03 PM

@newt10
Copy link
Collaborator Author

newt10 commented Sep 2, 2021

@epicfaace , this fixes a core problem with bootstrap4 form referenced in the issue #2007. Let me know if you need anything from me to move this along.

{children}
{displayLabel && rawDescription ? (
{labelComponent}
{shouldDisplayLabel && rawDescription ? (
Copy link
Member

Choose a reason for hiding this comment

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

Is this same problem present in other themes? I'm wondering because, for example, material-ui looks different: https://github.com/rjsf-team/react-jsonschema-form/blob/master/packages/material-ui/src/FieldTemplate/FieldTemplate.tsx#L50 (and label gen is directly in the textwidget -- see https://github.com/rjsf-team/react-jsonschema-form/blob/master/packages/material-ui/src/TextWidget/TextWidget.tsx)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It appears like Material-UI is also following a different approach than the core package. Here in the core package, field template owns the rendering of label (

)

Reviewing the documentation, the intended behavior is what is currently happening with core package.

My suggestion is that we go ahead with the fix for bootstrap 4 here and start creating tickets/issues to track this problem in other themes.

@newt10 newt10 requested a review from epicfaace September 2, 2021 19:18
@newt10 newt10 self-assigned this Sep 2, 2021
@newt10 newt10 added the bootstrap-4 bootstrap-4 related theme issue label Sep 2, 2021
@newt10 newt10 linked an issue Sep 2, 2021 that may be closed by this pull request
@newt10 newt10 added bug labelOrDescription Relating to labels and descriptions of fields in the form labels Sep 2, 2021
@newt10
Copy link
Collaborator Author

newt10 commented Sep 3, 2021

Can also provide workarounds for overriding TitleField and DescriptionField by allowing users to provide custom FieldTemplate. This has been requested many time under #1687 , #2219

@devo-wm
Copy link

devo-wm commented Sep 21, 2021

This would be a great add!

@heath-freenome
Copy link
Member

@newt10 If you were to update these changes on top of the v5 beta, I would be willing to review/approve. Please NOTE that the TextWidget was refactored into the new BaseInputTemplate so you will have to move your code accordingly

@heath-freenome heath-freenome added v5 refactor Needs refactor due to v5 breaking changes and removed awaiting review labels Aug 29, 2022
}: FieldTemplateProps) => {

const shouldDisplayLabel = displayLabel && (label || schema.title);
Copy link
Member

Choose a reason for hiding this comment

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

You can use the registry.schemaUtils.getDisplayLabel(schema, uiSchema) utility here.

@heath-freenome heath-freenome added the p1 Important to fix soon label Sep 28, 2022
heath-freenome added a commit to heath-freenome/react-jsonschema-form that referenced this pull request Oct 7, 2022
Fixed rjsf-team#2007

The current code for FieldTemplate and TextWidget in bootstrap 4 is incompatible with the core package. This breaks the
external facing API for customizing field template.
According to the core package implementation and documents, FieldTemplate is responsible for layout of labels, description, errors and help while the input widget along with the state is to be managed by the Field Component themselves (and Widgets)
If a user, uses custom field template with the bootstrap 4 form, the label is displayed multiple times, once by the custom field template (as intended in the API) and second time by the TextWidget (breaks from the core package convention)
This fixes the problem and updates the tests to catch any future regressions.

- Updated `FieldTemplate` to output `Label` generation, adding an additional check to simply render `children` when the component is `hidden`
- Updated `BaseInputTemplate`, `CheckboxesWidget`, `RadioWidget`, `SelectWidget` and `TextareaWidget`, removing the `Label` generation
- Updated the test snapshots
- Updated the `CHANGELOG.md` accordingly
heath-freenome added a commit to heath-freenome/react-jsonschema-form that referenced this pull request Oct 7, 2022
Fixed rjsf-team#2007

The current code for FieldTemplate and TextWidget in bootstrap 4 is incompatible with the core package. This breaks the
external facing API for customizing field template.
According to the core package implementation and documents, FieldTemplate is responsible for layout of labels, description, errors and help while the input widget along with the state is to be managed by the Field Component themselves (and Widgets)
If a user, uses custom field template with the bootstrap 4 form, the label is displayed multiple times, once by the custom field template (as intended in the API) and second time by the TextWidget (breaks from the core package convention)
This fixes the problem and updates the tests to catch any future regressions.

- Updated `FieldTemplate` to output `Label` generation, adding an additional check to simply render `children` when the component is `hidden`
- Updated `BaseInputTemplate`, `CheckboxesWidget`, `RadioWidget`, `SelectWidget` and `TextareaWidget`, removing the `Label` generation
- Updated the test snapshots
- Updated the `CHANGELOG.md` accordingly
heath-freenome added a commit that referenced this pull request Oct 7, 2022
Fixed #2007

The current code for FieldTemplate and TextWidget in bootstrap 4 is incompatible with the core package. This breaks the
external facing API for customizing field template.
According to the core package implementation and documents, FieldTemplate is responsible for layout of labels, description, errors and help while the input widget along with the state is to be managed by the Field Component themselves (and Widgets)
If a user, uses custom field template with the bootstrap 4 form, the label is displayed multiple times, once by the custom field template (as intended in the API) and second time by the TextWidget (breaks from the core package convention)
This fixes the problem and updates the tests to catch any future regressions.

- Updated `FieldTemplate` to output `Label` generation, adding an additional check to simply render `children` when the component is `hidden`
- Updated `BaseInputTemplate`, `CheckboxesWidget`, `RadioWidget`, `SelectWidget` and `TextareaWidget`, removing the `Label` generation
- Updated the test snapshots
- Updated the `CHANGELOG.md` accordingly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes bootstrap-4 bootstrap-4 related theme issue bug labelOrDescription Relating to labels and descriptions of fields in the form p1 Important to fix soon v5 refactor Needs refactor due to v5 breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bootstrap-4] Widget and FieldTemplate customisation
4 participants