-
Notifications
You must be signed in to change notification settings - Fork 16
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
337 multiple values field type #351
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 🎉
I've just added a few suggestions for minor improvements.
Also, should we remove the dynamic field from the design system?
src/sections/create-dataset/dynamic-fields-buttons/DynamicFieldsButtons.tsx
Outdated
Show resolved
Hide resolved
Thanks @MellyGray I made the changes you suggested. The e2e test is currently failing on get total dataset counts, which the clear test data PR should fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a failing e2e test. I'm not sure if this is a known error with an associated issue to fix it or if the error was introduced in this PR.
I have tested the create dataset form with the new multiple field in storybook and it looks good! But I have found some issues.
multiplefields.mov
-
After the initial load, why is it the only field that appears with a red border, if the others are also required?
-
After clicking the "add" button, to add one more author name, the other required fields show their red border.
-
I've been playing with "add" and "delete" buttons. You can see how after removing the second author name when we have three authors, the form is automatically submitted.
…in other input fields
Thanks @GPortas for finding that form submission bug! I fixed that, and updated the validation to use the same hook as the other fields, so now I think the behavior is consistent with the rest of the form. There is one small issue where if you add an author field, but keep it blank, the validation doesn't flag it, but I think that's ok because it is requiring at least one author, and also the validation will be changed when it is added to the dynamic fields version of the form. The e2e tests are failing because of race conditions when clearing test data, which I think will be fixed by this PR #360 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the addressed changes, @ekraffmiller
Maybe the following issues are not very relevant now because the field validation is going to change soon. Possibly not to address in this PR, but at least to keep in mind. Please let me know what you think:
The second author field does not show a required field message if it is left blank, anyway if we add a third author field and then remove it and leave the second blank, the required field message appears (I reproduce this in the recording below).
createdatasetformauthorfield.mov
if you add an author field, but keep it blank, the validation doesn't flag it, but I think that's ok because it is requiring at least one author
In JSF, if you add a multiple field that is required and leave it blank, the required field message is displayed. I think we should replicate this.
337 multiple values field type
What this PR does / why we need it:
Hard-coded version of multiple metadata values input, to be modified for the dynamic Create Dataset form
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: