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.

Sign up for GitHub

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

Jump to bottom

Update typescript definition of toIdSchema(), and passed the new idSeparator prop through to the AnyOfField and OneOfField inside of SchemaField #2743

Merged
merged 7 commits into from
Mar 3, 2022
Merged

Update typescript definition of toIdSchema(), and passed the new idSeparator prop through to the AnyOfField and OneOfField inside of SchemaField #2743

merged 7 commits into from
Mar 3, 2022

Conversation

heath-freenome
Copy link
Member

@heath-freenome heath-freenome commented Mar 2, 2022 •
edited
Loading

Reasons for making this change

Follow-up fix to #2628

  • Add the new idSeparator prop to the index.d.ts file for the toIdSchema() function definition
  • Also fixed the spelling of idPrefix parameter
  • Also passed idSeparator through to the AnyOfField and OneOfField in the SchemaField code
  • Updated ArrayField to also pass idPrefix and idSeparator down through to the inner SchemaField used for array field items
    • This includes adding a test to validate that it is properly being passed down now

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'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

@heath-freenome heath-freenome requested a review from epicfaace March 2, 2022 17:10
@heath-freenome
Follow-up fix to rjsf-team#2628
e7babd6 
- Add the new `idSeparator` prop to the `index.d.ts` file for the `toIdSchema()` function definition
- Also fixed the spelling of `idPrefix` parameter
@heath-freenome heath-freenome force-pushed the fix-toIdSchema-typescript-definition branch from 2643b4e to e7babd6 Compare March 2, 2022 17:12
epicfaace
epicfaace approved these changes Mar 2, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
epicfaace
epicfaace requested changes Mar 2, 2022
View reviewed changes
Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

Can you update the PR description / changelog as per this latest change?

@heath-freenome heath-freenome requested a review from epicfaace March 2, 2022 17:27
@heath-freenome
Copy link
Member Author

Can you update the PR description / changelog as per this latest change?

Done

@epicfaace epicfaace changed the title Follow-up fix to #2628 to update typescript definition of toIdSchema() Update typescript definition of toIdSchema(), and passed the new idSeparator prop through to the AnyOfField and OneOfField inside of SchemaField Mar 2, 2022
@epicfaace
Copy link
Member

Do we need to update the documentation at all (maybe where the AnyOfField / OneOfField props are documented)?

@heath-freenome
Copy link
Member Author

Do we need to update the documentation at all (maybe where the AnyOfField / OneOfField props are documented)?

I just scanned the docs for idPrefix (the sibling to idSeparator) and it isn't mentioned on those two fields. Actually, AnyOfField and OneOfField are only mentioned once, it the place where it describes fields you can override

@heath-freenome
- Updated ArrayField to also pass idPrefix and idSeparator down…
1ec339d 
… through to the inner `SchemaField` used for array field items

- Updated `CHANGELOG` again
- Reverted `package-lock.json` added in previous commit
@heath-freenome heath-freenome requested a review from epicfaace March 2, 2022 17:59
@heath-freenome
Copy link
Member Author

@epicfaace one more little fix... Isn't it interesting how we missed these in the original PR?

@heath-freenome
- Optimized ArrayField to get idPrefix and idSeparator directly…
9797dfd 
… from props instead of passing them through (following the pattern of `readonly` et al)
epicfaace
epicfaace reviewed Mar 2, 2022
View reviewed changes
packages/core/src/components/fields/ArrayField.js
@@ -819,6 +819,8 @@ class ArrayField extends Component {
uiSchema={itemUiSchema}
formData={itemData}
errorSchema={itemErrorSchema}
idPrefix={this.props.idPrefix}
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 an optimization or a bug fix? If the latter, maybe let's add a test to ensure it doesn't regress.

Copy link
Member Author

@heath-freenome heath-freenome Mar 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think it is closer to a bug fix as the props weren't being propagated all the way down... I can add a test

Copy link
Member Author

Choose a reason for hiding this comment

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

@epicfaace Test pushed... Did you want to reapprove or shall I merge?

epicfaace
epicfaace reviewed Mar 3, 2022
View reviewed changes
CHANGELOG.md Outdated

## @rjsf/core

- To improve performance, skip validating subschemas in oneOf / anyOf if formData is undefined (#2676)
- Fixed the `toIdSchema()` typescript definition to add new `idSeparator` prop
- Also passed the new `idSeparator` prop through to the `AnyOfField` and `OneOfField` inside of `SchemaField`
- Updated `ArrayField` in `@rjsf/core` to pass `idSeparator` and `idPrefix` through to `SchemaField`
Copy link
Member

Choose a reason for hiding this comment

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

Can you update this line to make it clearer that a bug was fixed?

Copy link
Member

Choose a reason for hiding this comment

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

@heath-freenome I think you forgot to do this

Copy link
Member Author

Choose a reason for hiding this comment

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

@heath-freenome I think you forgot to do this

It is there... the idPrefix={this.props.idPrefix} and idSeparator={this.props.idSeparator}

Copy link
Member

Choose a reason for hiding this comment

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

I meant, "update this line to make it clearer that a bug was fixed"

CHANGELOG.md

## @rjsf/core

- To improve performance, skip validating subschemas in oneOf / anyOf if formData is undefined (#2676)
- Fixed the `toIdSchema()` typescript definition to add new `idSeparator` prop
- Also passed the new `idSeparator` prop through to the `AnyOfField` and `OneOfField` inside of `SchemaField`
- Updated `ArrayField` in `@rjsf/core` to pass `idSeparator` and `idPrefix` through to `SchemaField`

Copy link
Member

Choose a reason for hiding this comment

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

Can you also update the PR title to note the 3 things that were changed?

@heath-freenome heath-freenome merged commit 00736f5 into rjsf-team:master Mar 3, 2022
@heath-freenome heath-freenome deleted the fix-toIdSchema-typescript-definition branch March 23, 2024 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epicfaace epicfaace epicfaace approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@heath-freenome @epicfaace