Remove unnecessary parameter #162
Conversation
|
@aditya81070 You need to follow the commit guidelines http://api.coala.io/en/latest/Developers/Writing_Good_Commits.html |
|
@PrajwalM2212 I will improve this in the future. Should I also change commit message for this time? |
|
@aditya81070 You would be on the safer side if you changed your commit this time itself . I don't think they will merge the PR until commit message is changed. |
|
@PrajwalM2212 I have changed the commit message. |
There was a problem hiding this comment.
@aditya81070 You have to make some changes. Firstly you can not have more than one commit in a single pull request and you have 3.You have to rebase all 3 commits by using git rebase -i HEAD~3 and then rebase in text editor.And secondly the commit message .Coala org is very strict in case of commit message.You must a 3 paragraph commit message .You can find a detailed desciption of commit message on https://api.coala.io/en/latest/Developers/Writing_Good_Commits.html . Its my personal suggestion you first read newcommer doc and git docs of coala.Please read fully https://api.coala.io/en/latest/Developers/Newcomers_Guide.html
There was a problem hiding this comment.
You need to squash all your comits into a single commit. Please refer to http://api.coala.io/en/latest/Developers/Git_Basics.html .
aditya81070
force-pushed
the
aditya81070-patch-1
branch
from
84818c1 to
29de489
Compare
|
@srivama @rishabhgarg25699 @PrajwalM2212 I have squashed three commits to one and also changed commit messages according to the Coala guidelines. |
| @@ -27,7 +27,7 @@ import {parseRoute, buildRoute} from './route-utils'; | |||
| const routes = [ | |||
| // Redirect from `/dashboard` to `/` | |||
| { path: '/dashboard', | |||
| onEnter: (state, replace) => browserHistory.push('/') | |||
There was a problem hiding this comment.
Could you please explain why have you removed state?
There was a problem hiding this comment.
In this route, if the user hit the /dashboard path in URL then he will be directed to /. so in this function, we are not changing any state of the component. so it was used but its value is not used.
There was a problem hiding this comment.
The work of redirecting is done by history.push() function.
There was a problem hiding this comment.
@shikharvaish28 please review the pull request.
There was a problem hiding this comment.
This is fine. However, the commit itself 3233746 has a problem. We should not use browserHistory here, but history
Line 5 in f08f19b
It's okay if you leave your change as it is, since it is a newcomer issue. After this gets merged we create a new issue to replace browserHistory with history.
|
@li-boxuan I did the commit message according to the guide. |
|
@li-boxuan What should I do now? |
aditya81070
force-pushed
the
aditya81070-patch-1
branch
from
29de489 to
44e8cb2
Compare
The two arguments `state` and `onEnter` not required. In the route when `/dashboard` matches, it is directed to `/` using browserHistory.push(). Fixes coala#137
aditya81070
force-pushed
the
aditya81070-patch-1
branch
from
44e8cb2 to
565f966
Compare
|
@li-boxuan I have changed the commit message |
|
This is not a bug so you should use |
|
@aditya81070 Are you still doing this? |
|
@aditya81070 , needs a manual rebase to resolve conflicts |
|
@jayvdb Okay I am doing this. |
|
Abandoned. |
fixes #137