-
Notifications
You must be signed in to change notification settings - Fork 18
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
31 - Create the form elements of the design system #56
31 - Create the form elements of the design system #56
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.
Overall this looks good. I did add some suggestions.
src/stories/ui/form/Form.stories.tsx
Outdated
<Form.GroupWithMultipleFields title="Author"> | ||
<Row> | ||
<Form.Group as={Col} controlId="basic-form-name"> | ||
<Form.Group.Label>Name</Form.Group.Label> | ||
<Form.Group.Input type="text" placeholder="Name" /> | ||
<Form.Group.Text> | ||
Please, specify the authors name. If there are multiple authors, please separate them | ||
with a comma. | ||
</Form.Group.Text> | ||
</Form.Group> | ||
<Form.Group as={Col} controlId="basic-form-surname"> | ||
<Form.Group.Label>Surname</Form.Group.Label> | ||
<Form.Group.Input type="text" placeholder="Surname" /> | ||
</Form.Group> | ||
</Row> |
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.
I'm not sure how accurate this is supposed to be but the way authors are presented is pretty different than what we do in reality. Maybe we should switch to a different example? Or make it more real? Maybe it's just a matter or changing Surname to Affiliation?
To add multiple authors, we use a plus sign to add more, like this:
Also, authors just have a single "Name" field with can be a person or an organization. So it's a little weird to see Name and Surname here (for user accounts we use Given Name and Family Name, by the way:
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.
When I did this I was just thinking of an example, not necessary a real one
Maybe I can think of a different example not related to Dataverse, because I would like to have all the different input types in one example
About the plus sign, should I add it to this PR as part of the form elements?
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.
I do think the plus sign is important to have somewhere. Yes, if you could add it, that would be great.
I'm happy to help make the examples a little more real. Otherwise, it's a bit confusing.
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.
I added the the plus button functionality and fixed the examples, let me know if I need to change something else
src/stories/ui/form/Form.stories.tsx
Outdated
</Row> | ||
<Row> | ||
<Form.Group as={Col} controlId="basic-form-description"> | ||
<Form.Group.Label>Description</Form.Group.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.
Authors don't have descriptions in Dataverse so this example feels weird.
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, sorry, I just wanted to show all the different input types in one example. I can think of a different example not related with Dataverse
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.
No worries. Thanks for taking a look!
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.
Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>
Yes. Something like this is what I had in mind: This is only one of the five tooltips, of course. And I'm not sure of the best way to add more space between "Affiliation" and the tooltip. 😅 This is what I'm doing:
Anyway, these tool tips are all over our forms so maybe we should go ahead and add them in this PR? I'm going to pull this out of "Ready for QA" (plenty of other stuff in the currently anyway), so we can decide how to proceed. Besides, it sounds like @MellyGray wants to add the plus button. I'll assign all three of us but please unassign yourself if you want off! 😄 |
@ekraffmiller @pdurbin I added the tooltips and fixed the plus button The plus button doesn't look exactly the same because this is how the Plus icon looks in Bootstrap 5: Let me know what you think |
@MellyGray the plus button looks good and I see tooltips too! Thanks! I haven't looked at the code yet. If @ekraffmiller is happy, this can go to QA. Or maybe I'll get to it. Thanks again! |
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 to me, thanks!
@MellyGray This looks good but there is a merge conflict. Would you please resolve this? Btw, on some of the Props, Group With Multiple Fields, when you click + or -, it does add a new instance but it also clears any existing instances of data. Is that something we would also see when on the page because that might cause some issues if people have not saved the page yet and wanted to add another entry. If this iteration is to get it roughly correct and adjust later, then np. Thanks! |
…-of-the-design-system 31 - Create the form elements of the design system
What this PR does / why we need it:
This PR add the form elements of the design system as described in the Design System Analysis Doc
Which issue(s) this PR closes:
Special notes for your reviewer:
I did't add all the form elements in the Dataverse because that could be infinite, I only added the ones we'll be using in the MVP.
For the tests I didn't test some FormElements independently because it didn't make sense to for example test the input component without the label component.
Suggestions on how to test this:
npm run storybook
Go to storybook and check all the different form elements in the Form doc file. You can even see the code from the storybook by clicking the code button in each story.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
None
Is there a release notes update needed for this change?:
Form elements of the design system added
Additional documentation:
https://docs.google.com/document/d/1OxCQO7B-yJaMWRl8EzuflcSmIaCmVSs19aoD83PjPmA/edit#heading=h.8nqu9gh25f0n