-
Notifications
You must be signed in to change notification settings - Fork 5
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
Implement a ContactForm component #5
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.
Any input component should expose input method. In this case, when it's a form, the methods are onChange or onFieldChange, or onInputChange, onSubmit.
Also, a component should be configurable: it must have a props for options, action, method and so on.
Please, always commit changes to |
Provide Reply-to field to contact form |
c7d5811
to
6bdb86e
Compare
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.
Improve component logic
.contact-form { | ||
background-color: $gray-100; | ||
position: relative; | ||
//padding: 3em 1em 1em; |
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.
Clean code comments up
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.
Other comments are not cleaned
components/contact-form/index.jsx
Outdated
{ value: 'field-three', label: 'Select field #3' }, | ||
] | ||
|
||
const ContactForm = ({ action, method = 'post', onCancel }) => ( |
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.
Provide more callbacks, at least onSubmit
pages/about/index.jsx
Outdated
> | ||
Send us a message | ||
</Button> | ||
{isToggleOn && ( |
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 usage Collapse
-component from Reactstrap instead of pure removing of the element.
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.
UncontrolledCollapse
is good but we have to control it from two different points so we have to use controlled one with state.
fe39753
to
edff28c
Compare
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.
Ask me if you have any question about the review.
components/contact-form/index.jsx
Outdated
{ value: 'field-three', label: 'Select field #3' }, | ||
] | ||
|
||
const ContactForm = ({ action, method = 'post', onCancel, onSubmit }) => ( |
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.
Pass options
through props
components/contact-form/index.jsx
Outdated
|
||
const ContactForm = ({ action, method = 'post', onCancel, onSubmit }) => ( | ||
<Form className="contact-form my-3" action={action} method={method}> | ||
<FormGroup className="pb-3" 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.
Always avoid utility classes
components/contact-form/index.jsx
Outdated
<option disabled selected> | ||
Please choose a subject | ||
</option> | ||
{options.map(option => ( |
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.
You can destructure option
directly to { label, value }
69fe2e5
to
c705b09
Compare
c705b09
to
3147799
Compare
* Update faq * add linking (#754) * fix missing buttons issue * feat: CORE-4429 open pdf in new tab * CORE-4429 babel config * update dependencies * [CORE-4431] docs-membership (#773) * update dataset text * CORE update api pdf file * CORE-4458 update terms page (#784) * update dataset text * CORE-4458: update terms page * temporary page * [bugfix] Config babel/runtime * Fix. Remove debug code (#794) * Fix/master content (#798) * HOTIFIX: search endpoint wasn't creating the right query * Master update from content (#795) * Update faq * add linking (#754) * fix missing buttons issue * feat: CORE-4429 open pdf in new tab * CORE-4429 babel config * update dependencies * [CORE-4431] docs-membership (#773) * update dataset text * CORE update api pdf file * CORE-4458 update terms page (#784) * update dataset text * CORE-4458: update terms page * temporary page * [bugfix] Config babel/runtime * Fix. Remove debug code (#794) --------- Co-authored-by: mcancellieri <mcancellieri@users.noreply.github.com> Co-authored-by: ekachxaidze98 <65679299+ekachxaidze98@users.noreply.github.com> Co-authored-by: Eka <ekachxaidze2@gmail.com> --------- Co-authored-by: Matteo Cancellieri <matteo.cancellieri@open.ac.uk> Co-authored-by: mcancellieri <mcancellieri@users.noreply.github.com> Co-authored-by: ekachxaidze98 <65679299+ekachxaidze98@users.noreply.github.com> Co-authored-by: Eka <ekachxaidze2@gmail.com> * [feature] Design v4.9.0. Membership component (#796) * Update faq * add linking (#754) * fix missing buttons issue * feat: CORE-4429 open pdf in new tab * CORE-4429 babel config * update dependencies * [CORE-4431] docs-membership (#773) * update dataset text * CORE update api pdf file * CORE-4458 update terms page (#784) * update dataset text * CORE-4458: update terms page * temporary page * [bugfix] Config babel/runtime * Fix. Remove debug code (#794) * [feature] Design v4.9.0. Membership component * Fix/master content (#798) * HOTIFIX: search endpoint wasn't creating the right query * Master update from content (#795) * Update faq * add linking (#754) * fix missing buttons issue * feat: CORE-4429 open pdf in new tab * CORE-4429 babel config * update dependencies * [CORE-4431] docs-membership (#773) * update dataset text * CORE update api pdf file * CORE-4458 update terms page (#784) * update dataset text * CORE-4458: update terms page * temporary page * [bugfix] Config babel/runtime * Fix. Remove debug code (#794) --------- Co-authored-by: mcancellieri <mcancellieri@users.noreply.github.com> Co-authored-by: ekachxaidze98 <65679299+ekachxaidze98@users.noreply.github.com> Co-authored-by: Eka <ekachxaidze2@gmail.com> --------- Co-authored-by: Matteo Cancellieri <matteo.cancellieri@open.ac.uk> Co-authored-by: mcancellieri <mcancellieri@users.noreply.github.com> Co-authored-by: ekachxaidze98 <65679299+ekachxaidze98@users.noreply.github.com> Co-authored-by: Eka <ekachxaidze2@gmail.com> --------- Co-authored-by: mcancellieri <mcancellieri@users.noreply.github.com> Co-authored-by: ekachxaidze98 <65679299+ekachxaidze98@users.noreply.github.com> Co-authored-by: Eka <ekachxaidze2@gmail.com> Co-authored-by: Matteo Cancellieri <matteo.cancellieri@open.ac.uk> * Netlify config #4 * Netlify config #5 --------- Co-authored-by: mcancellieri <mcancellieri@users.noreply.github.com> Co-authored-by: ekachxaidze98 <65679299+ekachxaidze98@users.noreply.github.com> Co-authored-by: Eka <ekachxaidze2@gmail.com> Co-authored-by: Matteo Cancellieri <matteo.cancellieri@open.ac.uk>
* Update faq * add linking (#754) * fix missing buttons issue * feat: CORE-4429 open pdf in new tab * CORE-4429 babel config * update dependencies * [CORE-4431] docs-membership (#773) * update dataset text * CORE update api pdf file * CORE-4458 update terms page (#784) * update dataset text * CORE-4458: update terms page * temporary page * [bugfix] Config babel/runtime * Fix. Remove debug code (#794) * Fix/master content (#798) * HOTIFIX: search endpoint wasn't creating the right query * Master update from content (#795) * Update faq * add linking (#754) * fix missing buttons issue * feat: CORE-4429 open pdf in new tab * CORE-4429 babel config * update dependencies * [CORE-4431] docs-membership (#773) * update dataset text * CORE update api pdf file * CORE-4458 update terms page (#784) * update dataset text * CORE-4458: update terms page * temporary page * [bugfix] Config babel/runtime * Fix. Remove debug code (#794) --------- Co-authored-by: mcancellieri <mcancellieri@users.noreply.github.com> Co-authored-by: ekachxaidze98 <65679299+ekachxaidze98@users.noreply.github.com> Co-authored-by: Eka <ekachxaidze2@gmail.com> --------- Co-authored-by: Matteo Cancellieri <matteo.cancellieri@open.ac.uk> Co-authored-by: mcancellieri <mcancellieri@users.noreply.github.com> Co-authored-by: ekachxaidze98 <65679299+ekachxaidze98@users.noreply.github.com> Co-authored-by: Eka <ekachxaidze2@gmail.com> * [feature] Design v4.9.0. Membership component (#796) * Update faq * add linking (#754) * fix missing buttons issue * feat: CORE-4429 open pdf in new tab * CORE-4429 babel config * update dependencies * [CORE-4431] docs-membership (#773) * update dataset text * CORE update api pdf file * CORE-4458 update terms page (#784) * update dataset text * CORE-4458: update terms page * temporary page * [bugfix] Config babel/runtime * Fix. Remove debug code (#794) * [feature] Design v4.9.0. Membership component * Fix/master content (#798) * HOTIFIX: search endpoint wasn't creating the right query * Master update from content (#795) * Update faq * add linking (#754) * fix missing buttons issue * feat: CORE-4429 open pdf in new tab * CORE-4429 babel config * update dependencies * [CORE-4431] docs-membership (#773) * update dataset text * CORE update api pdf file * CORE-4458 update terms page (#784) * update dataset text * CORE-4458: update terms page * temporary page * [bugfix] Config babel/runtime * Fix. Remove debug code (#794) --------- Co-authored-by: mcancellieri <mcancellieri@users.noreply.github.com> Co-authored-by: ekachxaidze98 <65679299+ekachxaidze98@users.noreply.github.com> Co-authored-by: Eka <ekachxaidze2@gmail.com> --------- Co-authored-by: Matteo Cancellieri <matteo.cancellieri@open.ac.uk> Co-authored-by: mcancellieri <mcancellieri@users.noreply.github.com> Co-authored-by: ekachxaidze98 <65679299+ekachxaidze98@users.noreply.github.com> Co-authored-by: Eka <ekachxaidze2@gmail.com> --------- Co-authored-by: mcancellieri <mcancellieri@users.noreply.github.com> Co-authored-by: ekachxaidze98 <65679299+ekachxaidze98@users.noreply.github.com> Co-authored-by: Eka <ekachxaidze2@gmail.com> Co-authored-by: Matteo Cancellieri <matteo.cancellieri@open.ac.uk> * Netlify config #4 * Netlify config #5 * Netlify config #6 --------- Co-authored-by: mcancellieri <mcancellieri@users.noreply.github.com> Co-authored-by: ekachxaidze98 <65679299+ekachxaidze98@users.noreply.github.com> Co-authored-by: Eka <ekachxaidze2@gmail.com> Co-authored-by: Matteo Cancellieri <matteo.cancellieri@open.ac.uk>
No description provided.