-
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
Dynamically render form field in Create Dataset Form + Fields Validations #369
Dynamically render form field in Create Dataset Form + Fields Validations #369
Conversation
…Item html attributes
…d extend accordion html attributs and props
…ions buttons space
… feature/336-dynamically-render-form-fields
…-fields Feature/336 dynamically render form fields
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.
Works great! 🎉
I left some comments for minor improvements. Also can you solve the merge conflicts?
packages/design-system/src/lib/components/accordion/Accordion.tsx
Outdated
Show resolved
Hide resolved
packages/design-system/src/lib/components/accordion/Accordion.tsx
Outdated
Show resolved
Hide resolved
cy.findByText('Author') | ||
.closest('.row') | ||
.within(() => { |
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.
Consider using floating labels to look for the inputs by their labels
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 is only for composed fields, where for example in this case, 'Author' is the root 'label' lets say but is not actually a label.
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.
Yes, I understand. That's why I was suggesting the use of Floating Labels because each input needs to have its own label. "Author" cannot be the label for four different inputs.
We can use floating labels to provide each input with its own label "Author name", "Author Affiliation", without significantly altering the UI, as these labels are floating.
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.
Oh, I understand now what you mean, labels from composed fields groups may not be unique. Let discuss it with the team, great!
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.
@GermanSaracca I think that change doesn't need to block this PR. I'll approve the PR, and you can discuss it with the team. If there is an action item, you can create an issue. Thanks!
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.
All requested changes addressed except for the multiple fields labels. Take a look at my comment #369 (comment)
We can discuss this decision with the team
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!
Looks good, merging! |
…ds-and-validation Dynamically render form field in Create Dataset Form + Fields Validations
What this PR does / why we need it:
Dynamically renders all fields of each metadatablock in the dataset creation form and adds their respective validations.
This sub branch is a combination of two issues that are tightly related.
Both issues have their own pr's merged to this sub branch:
Which issue(s) this PR closes:
Dynamically render Create Dataset Form fields based on the backend configuration #336
Add validation library to validate fields from the Create Dataset Form #338
Special notes for your reviewer:
Create Dataset Form Storybook
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
Run unit and e2e tests and check they are all passing.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Yes.
Also I modified at spa level not design system level the styles for the checkbox group, I think if it is successful we could apply it to the design system directly in the future.
This checkbox group that allows to select multiple values is a temporary replacement of the select multiple not yet done.
Examples below: 👇👇
Is there a release notes update needed for this change?:
Additional documentation:
I have refactor some design system components:
bsPrefix
andas
props and extend html props attributes.onClick
,bsPrefix
andas
props and extend html props attributes.bsPrefix
andas
props and extend html props attributes.All tests were ok and add one more test to test the cloning through childrens wrapped in fragments.