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 id of anyof select #1212

Merged
merged 7 commits into from
Dec 23, 2019
Merged

Fix id of anyof select #1212

merged 7 commits into from
Dec 23, 2019

Conversation

epicfaace
Copy link
Member

@epicfaace epicfaace commented Mar 7, 2019

This fixes a change that was done in #1118. It seems like the id should be suffixed with a double underscore, not a single underscore.

For example, see examples here (https://github.com/mozilla-services/react-jsonschema-form/blob/master/src/components/fields/ArrayField.js#L27)

@epicfaace epicfaace requested a review from LucianBuzzo March 7, 2019 00:24
@epicfaace
Copy link
Member Author

@LucianBuzzo could you please review this?

@epicfaace epicfaace requested a review from edi9999 March 27, 2019 13:40
Copy link
Collaborator

@edi9999 edi9999 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 add a test to make sure this does not regress ?

@epicfaace
Copy link
Member Author

@edi9999 I just changed the id, so I'm not sure how a test could meaningfully cover this change; what kind of test do you have in mind?

@edi9999
Copy link
Collaborator

edi9999 commented Apr 3, 2019

I mean a test like here : https://github.com/mozilla-services/react-jsonschema-form/blob/master/test/BooleanField_test.js#L40

Where you render that component and verify that the id is the expected one.

Copy link
Collaborator

@LucianBuzzo LucianBuzzo left a comment

Choose a reason for hiding this comment

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

This looks good to me, can you please add a test that checks that the ID is correct?

@epicfaace
Copy link
Member Author

I've added tests -- but I realized that both the oneof and anyof widgets have the same id (i.e., "__anyof_select" instead of "__oneof_select" and "__anyof_select"). Was this intentional or do you think we should fix this as well?

@edi9999
Copy link
Collaborator

edi9999 commented Jun 18, 2019

Good catch, I think it is not intentional and we should fix it.

@epicfaace
Copy link
Member Author

This is technically a backwards incompatible change anyway, so let's just add this in v2.

@epicfaace epicfaace requested a review from edi9999 November 25, 2019 22:16
@epicfaace epicfaace requested a review from LucianBuzzo December 1, 2019 23:28
@epicfaace
Copy link
Member Author

@LucianBuzzo / @edi9999 could you review?

@epicfaace epicfaace merged commit 53ee91f into master Dec 23, 2019
@epicfaace epicfaace deleted the epicfaace-patch-1 branch December 23, 2019 11:32
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.

3 participants