-
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
Multiple dynamic fields - Create Dataset Form #395
Multiple dynamic fields - Create Dataset Form #395
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.
This looks good and works very well!
A general comment I have is, could we MetadataBlockFormFields and the components within it to a shared folder? I think that code will end up being used in the Edit Dataset action, and also it seems like code that other UIs would want to reuse. Also creating stories and tests for these components I think would help with re-use. What do you think?
Hi @ekraffmiller I thought the same thing at the time, but I think it is not yet ready to be in a shared component, it will have to be more generic yet, but indeed that should be the direction we have to follow, maybe when the time comes to make the edit form we could simply make the necessary changes to make it fully functional for both create and edit and then put it in shared folder, what do you think? |
Ok, That makes sense! We will have a better idea how to refactor it when we do the edit page. |
I noticed the e2e tests are failing, maybe we can merge the latest from develop when the race condition PR is merged, to make sure the tests are ok. |
Yes, I think that will be the safest thing to do. |
@ekraffmiller all e2e tests are passing, I only update the test for the create dataset form to select an option from the select multiple field. |
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. approved!
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 and works great!
qa_multiple_dynamic_fields.mov
…ypes-to-create-dataset-form Multiple dynamic fields - Create Dataset Form
What this PR does / why we need it:
Adds the ability to add multiple values to a field.
Which issue(s) this PR closes:
Closes #376
Special notes for your reviewer:
required
andwithinMultipleFields
props being cloned to all childrens inside because did not work correctly when we had nested elements to clone within the Forms Group (e.g. adding props to all elements unnecessarily).withinMultipleFields
props from form elements and with it the<FormElementLayout/>
, so now the design system is not responsible for rendering the form group as a column or as a row.In some cases within the Create Dataset form when working on this issue I found some complications when working with so many dynamic fields.
I think it's pretty useful to just pass a prop to render the form group one way or another, but I think the use of
react.cloneElement
was being counter-intuitive in this case.Perhaps we could see in a separate issue a way to accomplish the same thing but without the use of
cloneElement
.Suggestions on how to test this:
Step 1: Run the Development Environment
npm i
.cd packages/design-system && npm run build
.cd ../../
..env
file similar to.env.example
, with the variableVITE_DATAVERSE_BACKEND_URL=http://localhost:8000
.cd dev-env
../run-env.sh unstable
.Step 2: Test the Create Dataset Form
Is there a release notes update needed for this change?:
**Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Yes.
Additional documentation:
No.