Skip to content
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.

Sign up for GitHub

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

Jump to bottom

#39 SignUp Component #43

Merged
merged 5 commits into from
Feb 23, 2021
Merged

#39 SignUp Component #43

merged 5 commits into from
Feb 23, 2021

Conversation

Kathuria
Copy link
Contributor

@Kathuria Kathuria commented Feb 7, 2021

Sign Up component #39

Fixes: #39

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Applying Component in App to render SignUp

Checklist:

  • My code follows the style guidelines of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.

Screenshots

SignUp

@welcome
Copy link

welcome bot commented Feb 7, 2021

Hello there!👋 Welcome to the project!💖⚡

Thank you and congrats🎉 for opening your very first issue in this project. We at hackStation provide a platform to share your work in number. Please adhere to our Code of Conduct.🙌 Kindly ensure you have fulfilled all the project guidelines.

ashishnagpal2498
ashishnagpal2498 requested changes Feb 9, 2021
View reviewed changes
Copy link
Owner

@ashishnagpal2498 ashishnagpal2498 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No packages should be added. Come up with a better design.

frontend/src/components/layout/SignUp.js Outdated
<div className="container">
<div className="title">Sign Up</div>
<div className="body">
<input type="text" name="name" placeholder="Name"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Come up with a better design. This is too simple. The issue is "Medium level" on purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, Will incorporate comments & try for better design. Do you have ref/design thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
Updated Design

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashishnagpal2498 : Added newly designed SignUp Component, have a look.

@Kathuria
#39 Sign Up Component Revised
4244179 
Incorporated Review Comments
#1 Removed react-toastify package
#2 Remove toast Errors
#3 Complete Revamp Design
ashishnagpal2498
ashishnagpal2498 reviewed Feb 12, 2021
View reviewed changes
frontend/src/components/styles/SignUp.css Outdated
@import url('https://fonts.googleapis.com/css2?family=Poppins:wght@100;200;400;500;600;700&display=swap');

:root{
--red-bg-color: #ff7a7a;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you copied code from somewhere?

Copy link
Contributor Author

@Kathuria Kathuria Feb 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, any issues in that as font suits this component. Can I share credits?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an issue with that. But please refine the code.
Don't use properties on :root. Reduce redundant properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashishnagpal2498 : Done, Please check now.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InkedsignupComp_LI
Why do you need this when there's already a button.
Also, instead of state make a route for " SignUp ".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashishnagpal2498 : Done, Please have a look - I haven't used existing icon as it was mapped to login - now updated to Signup

@Kathuria
#39 Code Refining
1e3a87b 
Cleaned Up
@ashishnagpal2498 ashishnagpal2498 added Medium SWOC Script Winter of Code UI/UX labels Feb 14, 2021
@Kathuria
#39 Added SignUp as Route path
221d22d 
Incorporated review comments
@ashishnagpal2498
Copy link
Owner

Please resolve merge conflicts.

@Kathuria
Copy link
Contributor Author

Please resolve merge conflicts.

Done

@ashishnagpal2498 ashishnagpal2498 merged commit 43816c8 into ashishnagpal2498:master Feb 23, 2021
@welcome
Copy link

welcome bot commented Feb 23, 2021

Congrats on merging your first pull request! 🎉 All the best for your amazing open source journey ahead 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashishnagpal2498 ashishnagpal2498 ashishnagpal2498 requested changes

Assignees
No one assigned
Labels
Medium SWOC Script Winter of Code UI/UX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sign Up component
2 participants
@Kathuria @ashishnagpal2498