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 #3608 by removing ternary and ensuring AntD FieldTemplate wraps root in Form.Item #3621

Merged
merged 2 commits into from
Apr 22, 2023

Conversation

nickgros
Copy link
Contributor

  • Ensure the root field is always wrapped in Form.Item

The original ternary expression has existed since the antd theme was added in #1561. I am not familiar enough with antd to know what, if any, undesirable effects this change could cause. I looked at most of the playground examples and did not notice any new issues.

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests. I've run 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

- Ensure the root field is always wrapped in Form.Item

The original ternary expression has existed since the antd theme was added in rjsf-team#1561. I am not familiar enough with antd to know what, if any, undesirable effects this change could cause.
Comment on lines -97 to -99
{id === 'root' ? (
children
) : (
Copy link
Contributor Author

@nickgros nickgros Apr 22, 2023

Choose a reason for hiding this comment

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

The reason the error didn't show up is because the example in #3608 had a String field as the root, and the error rendering is handled by Form.Item. Since there was no Form.Item, there was no visible error message.

@heath-freenome
Copy link
Member

Nice fix. Have you figured out what is causing #3609?

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Heath C <[email protected]>
@nickgros
Copy link
Contributor Author

Nice fix. Have you figured out what is causing #3609?

An internal call to rc-util findDOMNode but React is undefined so node instanceof React.Component incorrectly returns undefined. Might be a bundling issue? https://github.com/react-component/util/blob/master/src/Dom/findDOMNode.ts

@heath-freenome
Copy link
Member

heath-freenome commented Apr 22, 2023

Actually this fix could also eliminate the need for the label changes I made to Select (and maybe Checkboxes and Radiobutton) since the label is output as part of the item. And the bug was related to the root item not having labels. Meaning that #3594 can likely be fixed by reverting my changes to those components

@nickgros
Copy link
Contributor Author

The extra TitleField for the SelectWidget can definitely be removed. I think the other widgets did need the new label added in your change, but I'll double-check

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.

2 participants