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

fix(select/radio): Accept just the values in options (plus '' and null for backward-compatibility) #18

Merged
merged 19 commits into from
Jul 3, 2023
Merged

fix(select/radio): Accept just the values in options (plus '' and null for backward-compatibility) #18

merged 19 commits into from
Jul 3, 2023

Conversation

gabrielseco
Copy link
Collaborator

@gabrielseco gabrielseco commented Jun 22, 2023 •
edited by sandrina-p
Loading

1. Description

Fix select and radio yup schemas to only validate the values provided by a json schema

2. How to test this

I created a playground with vite where you can test the different fields like select and radio but I think we should remove it from this PR and moved it to a new one to have a proper release with a good developer experience.

The playground demo is based on the codesandbox from our docs

PS: I'll be on PTO from tomorrow, I'll be back on the 3th of July

Edit by @sandrina-p:

  • I've added more automated tests to cover all the scenarios.
  • I've recorded a public Loom explaining why we still accept empty strings and null. For any questions, feel free to comment directly in this PR or open a new issue.

@gabrielseco gabrielseco self-assigned this Jun 22, 2023
@sandrina-p sandrina-p mentioned this pull request Jul 2, 2023
Closed
@sandrina-p sandrina-p changed the title fix(select) - fix select and radio field validations to only accept the values given by json-schemas instead of any string fix(select/radio): Accept only values included in options (and if optional, also empty strings) Jul 2, 2023
@sandrina-p sandrina-p self-assigned this Jul 2, 2023
@sandrina-p sandrina-p mentioned this pull request Jul 2, 2023
Merged
sandrina-p
sandrina-p reviewed Jul 3, 2023
View reviewed changes
src/yupSchema.js Outdated Show resolved Hide resolved
@sandrina-p sandrina-p force-pushed the rmt-23-jsf-select-validation-is-incomplete branch from 58e0efc to 93fbd2e Compare July 3, 2023 08:22
brennj
brennj reviewed Jul 3, 2023
View reviewed changes
Copy link
Collaborator

@brennj brennj left a comment

Choose a reason for hiding this comment

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

Still going through this - code looks fine for now but also going through the failures we have internally against the pipeline

src/tests/createHeadlessForm.test.js Show resolved Hide resolved
src/yupSchema.js Outdated Show resolved Hide resolved
src/yupSchema.js Outdated Show resolved Hide resolved
brennj
brennj reviewed Jul 3, 2023
View reviewed changes
src/yupSchema.js Outdated Show resolved Hide resolved
@sandrina-p sandrina-p changed the title fix(select/radio): Accept only values included in options (and if optional, also empty strings) fix(select/radio): Accept only values included in options (and '' null for retrocompatibility) Jul 3, 2023
@sandrina-p sandrina-p changed the title fix(select/radio): Accept only values included in options (and '' null for retrocompatibility) fix(select/radio): Accept just the values in options (plus '' and null for backward-compatibility) Jul 3, 2023
@sandrina-p sandrina-p force-pushed the rmt-23-jsf-select-validation-is-incomplete branch from 6c7c1d7 to eaabaf7 Compare July 3, 2023 20:30
@brennj
Copy link
Collaborator

brennj commented Jul 3, 2023

Approving this one based on passing pieplines internally

@sandrina-p sandrina-p merged commit 37501d2 into main Jul 3, 2023
3 checks passed
@sandrina-p sandrina-p deleted the rmt-23-jsf-select-validation-is-incomplete branch July 3, 2023 22:02
sandrina-p
sandrina-p reviewed Jul 4, 2023
View reviewed changes
src/yupSchema.js

We'd need to implement a feature_flag/transition deprecation warning
to give devs the time to adapt their integrations before we fix this behavior.
Check the PR#18 and tests for more details.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gabrielseco @brennj As promised, I've recorded a public loom (and added it to the PR description) explaining this workaround with examples.

Hopefully, it will help you better understand/explain this in the future when we come back to this to and create the migration phase to stop accepting "" and null when we shouldn't.

@sandrina-p sandrina-p mentioned this pull request Jul 18, 2023
Merged
@sandrina-p sandrina-p mentioned this pull request Aug 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dilvane dilvane dilvane left review comments

@sandrina-p sandrina-p sandrina-p left review comments

@brennj brennj brennj approved these changes

Assignees

@gabrielseco gabrielseco

@sandrina-p sandrina-p

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gabrielseco @brennj @dilvane @sandrina-p @gabrielremote