Refactor Fix #80
Refactor Fix #80
Conversation
|
Deploy preview for footsteps-app ready! Built with commit 715d9d6 |
|
There's only one check left now. You can do it! @R3l3ntl3ss What do you think of this PR? There is a massive change in the structure of the project. |
Please have a look at the problem and help me with this 🤠... |
|
@xlogix @R3l3ntl3ss Here it is showing that this PR has conflicts but I don't have an option to resolve them. The option for resolving the commits is not available. Why is this so and how can I resolve the commits ? |
|
Hey @vngarg, you'll have to resolve it using the cmd line. |
|
@xlogix Actually, I had changed the location of these files in this PR. |
|
I understand that. But, right now it will show it as conflicts until you merge the master branch. Slack |
There was a problem hiding this comment.
Hey @vngarg. Did you perform tests for all the files that you made changes to?
I am getting this error while creating a path.
Please test all your changes before you create a pull request. Since you are changing a lot of files please perform tests for the whole app and ensure nothing is broken.
Also, I see that you have broken down the components to stateful and stateless folders. Are you sure we need this? When we created the app we structured the components based on the pages on the app. Could you explain why the approach you have taken would be better?
Thanks for your contribution.
|
@prajwal714 Since you created the issue, could you explain why this approach would be better for us? |
Sorry, some of the tests might have been left untested .... |
There are several redundant components in the files, which can be reused if they are broken down into separate components. Also, it gives the project a unified structure since the components will be reused in other future pages as well. Refactoring the code was necessary else it would eventually lead too a spaghetti codebase. @xlogix @R3l3ntl3ss what we can do for now is instead of completely changing the structure proceed part wise, modularizing each page first. It will lead to less conflicts and the transition would be smooth. |
|
I totally agree with that. But do we really need to categorize the components based on the State of the component? Right now the components are arranged based on their function and page, I am not sure if changing up that is necessary. |
|
I agree with @R3l3ntl3ss on keeping components based on their page/function. This helps to navigate the project easily. |
|
@prajwal714 Yeah, we should proceed page-wise. Modularising the components as we encounter them. @vngarg Are you okay with that? You can raise pull requests each time you modularise. I hope we can complete the whole transition in a few days? |
@xlogix @R3l3ntl3ss @prajwal714 I don't have any problem with this .... |
| err_msg = "Please fill the path details" | ||
| return false | ||
| } else if (footstep.footsteps.length < 1) { | ||
| err_msg = "Please add a few footsteps. Footsteps can not be empty." |
There was a problem hiding this comment.
Lets put error messages in a separate file and import them as a CONST.
It will help us manage the errors at a single place.
| noContent = footsteps => { | ||
| let valid = false | ||
|
|
||
| for (var i = 0; i < footsteps.length; i++) { |
There was a problem hiding this comment.
Avoid using var. It leads to memory leak issues. use let
for (let i = 0; i < footsteps.length; i++) {
| <img | ||
| className={styles.menuProfileImg} | ||
| className={Menustyles.menuProfileImg} | ||
| src={user.profile_pic} | ||
| alt="" |
There was a problem hiding this comment.
Add a alt property like "Profile Picture"
I had refactored the code and tried to remove the prolems..
It closes the issue #68