-
Notifications
You must be signed in to change notification settings - Fork 77
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
Login ui for the website #28
Conversation
@abha224 @sidvenu
Please clarify these! |
@bismitaguha the issues are related to each other. When a button is clicked, browsers could interpret it differently to be a submit button (whereas it is a button to run a piece of code, and then use code to navigate away), and thus your Try adding |
As we talked about in the weekly session, please do try to give separate PRs for each issue. It's hard to go through 22 files with about 1000 LoC additions. Within a single PR, do break it down into logically separate commits. And please follow the commit message style guide: https://github.com/anitab-org/mentorship-android/wiki/Commit-Message-Style-Guide. Is it possible for you to change the commit messages to follow the style guide? |
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.
Please do change the commit messages as per the comment above.
@sidvenu I was suggested by @Monal5031 to proceed to further issues and rebase later before merging. So I am doing the same. I hope it's fine. |
@bismitaguha If that is the case, sure, we can use the message style guide while merging. But please do keep the above suggestions in mind for your next PRs 😄. |
I have made separate PRs based on your suggestions in the meeting. I will look into the commit messages. 😊 |
Amazing. Let us know if the above solution to the |
@bismitaguha I think there was a miscommunication, I suggested to start working on a separate branch so that would help you save time while the main PR is in review. I didn't meant to add the changes in same PR |
This is in a separate branch. The previous PR #27. Once the previous one gets merged I will rebase this. I did that same with the previous issue . It was in backend repo. I hope that's fine?? @Monal5031 |
@Monal5031 if two issues are heavily dependent on each other that it'll hinder developement of one without the other, then absolutely - we can have a single PR for both the issues. If that is not the case, I feel separate PRs would be ideal - since it is just good open source practice and it prevents big PRs to a certain extent. What do you think? |
Agreed @sidvenu makes total sense |
@bismitaguha Im not able to reproduce the error? can you provide the steps again? |
src/components/Login.js
Outdated
@@ -28,7 +28,7 @@ class Login extends Component { | |||
}) | |||
|
|||
submitLogin = () => { | |||
|
|||
console.log("hello") |
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.
remove this line
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.
Also, please add new lines to the files you've created in the current PR that doesn't have a new line yet.
package.json
Outdated
"devDependencies": { | ||
"prettier": "2.0.5" | ||
} |
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.
Can we move this out of this PR? I've just created an issue - #34 for using prettier. Reason being we may need to format code that is outside the scope of this PR as well.
src/components/Navbar.js
Outdated
// if(localStorage.getItem('token') !== null){ | ||
this.props.getInfo() | ||
this.props.getUser() | ||
// } | ||
|
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.
Comment to be removed?
@bismitaguha can you resolve the conflicts? |
Add Routing Add Registration component, actions & reducers
Modify routing Modify Login action
Modify form validation
@abha224 Merge conflicts resolved! |
Description
Fixes #16
Type of Change:
Code/Quality Assurance Only
How Has This Been Tested?
Currently requires double click to enter Dashboard Page on successful Login
Checklist:
Code/Quality Assurance Only