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.

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

Signup Page for Fabrik #502

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Conversation

sayamkanwar
Copy link

Hi,
I have created the signup page for Fabrik. I have added all the custom validation code. For csrf, I have used npm package django-react-csrftoken. The signup form should be available at /#/signup.

Here's a demo

fabrik_signup

Please review @Ram81 @RishabhJain2018 @yashdusing @utsavgarg
Thank you! :)

Copy link
Member

@Ram81 Ram81 left a comment

Choose a reason for hiding this comment

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

@sayamkanwar can you please post some other mock ups for signup page, this doesn't match the theme of Fabrik.

@sayamkanwar
Copy link
Author

Sure @Ram81

@sayamkanwar
Copy link
Author

@Ram81, do you want me to completely change the design or change the color scheme to match with that of Fabrik?

@sayamkanwar
Copy link
Author

Or would you like something like this? @Ram81

screenshot 2018-12-11 at 4 01 33 pm

@sayamkanwar
Copy link
Author

@Ram81, Can you please describe what type of design are you expecting?

@sayamkanwar
Copy link
Author

Also @Ram81, can you please review the code as well? So that I can do all the changes at once.
Thank you!

@Ram81
Copy link
Member

Ram81 commented Dec 11, 2018

@sayamkanwar you can see some work done by previous students who attempted the task there was one PR by @c0derlint which had a good design example

@sayamkanwar
Copy link
Author

sayamkanwar commented Dec 11, 2018

Oh okay @Ram81. But he had combined the login and signup form right? For me, I have to just create a sign up form?

@sayamkanwar
Copy link
Author

Is this fine @Ram81?

new_signup_mockup

Copy link
Member

@Ram81 Ram81 left a comment

Choose a reason for hiding this comment

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

@sayamkanwar in your current implementation how are you ensuring the security of password, I haven't worked on login functionality in react yet. Are there any standard methods used to handle password security in UI component ?

And can you also share some alternative mock ups too, this one is good but to me it doesn't seem fitting to Fabrik's theme.

this.state = {
error_username: 0,
error_email: 0,
error_password1: 0,
Copy link
Member

Choose a reason for hiding this comment

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

@sayamkanwar please rename this variables, try not to use 1 or 2 with variable for multiple variables for same purpose

Copy link
Author

Choose a reason for hiding this comment

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

Actually I was trying to maintain consistency as these variables were used in /accounts/signup/. So should I still change these?
screenshot 2018-12-11 at 11 01 11 pm

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Copy link
Author

Choose a reason for hiding this comment

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

Okay

).style.borderColor = this.state.wrong;
let email_label = document.getElementById("email-label");
if (email != "") {
email_label.style.fontSize = "0.7em";
Copy link
Member

Choose a reason for hiding this comment

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

For style changes based on actions can't you create separate css class json in the component and change the style class of component ?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'll keep this in mind when coding the new design.

this.setState({ error_email: 0 });
}
}
validatePassword1() {
Copy link
Member

Choose a reason for hiding this comment

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

Rename methods based on logic in method

Copy link
Author

Choose a reason for hiding this comment

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

But there are 2-3 checks inside the method so how do I rename it?

Copy link
Member

@Ram81 Ram81 Dec 11, 2018

Choose a reason for hiding this comment

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

based on the type of checks ex: validateEmailPassword, etc

Copy link
Author

Choose a reason for hiding this comment

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

Umm so if my validatePassword function has 2 checks for empty string and maximum char length so I should name it as checkPasswordEmptyMaxChar?

"csrfmiddlewaretoken"
)[0].value;
if (
this.state.error_username == 0 &&
Copy link
Member

Choose a reason for hiding this comment

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

Can you correct the indentation of this if () statement

Copy link
Author

Choose a reason for hiding this comment

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

Sure

password2: password2
},
success: function(result) {
console.log(result);
Copy link
Member

Choose a reason for hiding this comment

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

Remove debugging logs

Copy link
Author

Choose a reason for hiding this comment

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

Sure

let success = document.getElementsByClassName("success")[0];
let rightImg = document.getElementsByClassName("graphic")[0];
let logo = document.getElementsByClassName("logo")[0];
setTimeout(function() {});
Copy link
Member

Choose a reason for hiding this comment

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

Add newlines to make code clean

Copy link
Author

Choose a reason for hiding this comment

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

Okay

@sayamkanwar
Copy link
Author

@Ram81, By security do you mean the prevention for different types of attacks through the input?

@Ram81
Copy link
Member

Ram81 commented Dec 11, 2018

@sayamkanwar yes

@sayamkanwar
Copy link
Author

Okay @Ram81, I'll add those checks. Thanks for pointing out! :)

@sayamkanwar
Copy link
Author

sayamkanwar commented Dec 11, 2018

Do these 2 match Fabrik's theme? @Ram81

new_signup_mockup2

new_signup_mockup3

If you like these, please mention your choice as well.

@Ram81
Copy link
Member

Ram81 commented Dec 12, 2018

@sayamkanwar second one looks good for now

@sayamkanwar
Copy link
Author

@Ram81, okay I'll code this then.
Thank you! :)

@sayamkanwar
Copy link
Author

I have created the new design with all the changes you had asked for. I have also added the code to clean the string before submitting to the database which will prevent cyber security attacks. Also added all types of validation rules in the form. I have also added the code which gives an error if the account with the provided details already exists in the database.

New UI
screenshot 2018-12-12 at 9 22 39 pm

Demo
demo_account

Please review @Ram81 @RishabhJain2018
Thank you! :)

@sayamkanwar
Copy link
Author

Also, if you see any odd indentations, that's probably because of ESLint. Without those, I was getting webpack errors and warnings.

},
success: function(result) {
if (result.location == "/") {
window.location = "/";
Copy link
Member

Choose a reason for hiding this comment

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

@sayamkanwar we would want to set the logged in user details in user login component but we would want it to be done in a secure way can you try to figure out some method for it

Copy link
Author

Choose a reason for hiding this comment

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

Sure @Ram81, I'll look into it.

@sayamkanwar
Copy link
Author

@Ram81,
I have added the code which sets the logged in user's details in localStorage. localStorage will be the best option to maintain such type of records which are to be used at the frontend for real-time collaboration purpose.

Here's a demo on how this will work.
signup-localstorage

So whenever we need to use them, we can simply read the item value from localStorage through javascript.
For User ID: localStorage.getItem("userID"),
For Username: localStorage.getItem("username"),
For Email: localStorage.getItem("email")

Let me know your opinion on this.
Thank you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants