footsteps-app icon indicating copy to clipboard operation
footsteps-app copied to clipboard

Published 20 hours ago • verified publisher icon fnplus

Refactor Fix

Open vngarg opened this issue 4 years ago • 16 comments

I had refactored the code and tried to remove the prolems.. It closes the issue #68

vngarg avatar Mar 06 '20 08:03 vngarg

Deploy preview for footsteps-app ready!

Built with commit 715d9d644aca9c0ebef3348882631f571b2ca3f4

https://deploy-preview-80--footsteps-app.netlify.com

netlify[bot] avatar Mar 06 '20 08:03 netlify[bot]

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.

xlogix avatar Mar 06 '20 12:03 xlogix

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 🤠... I had tried alot 😔

vngarg avatar Mar 06 '20 13:03 vngarg

@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 ?

vngarg avatar Mar 08 '20 09:03 vngarg

Hey @vngarg, you'll have to resolve it using the cmd line. Git Cmd

xlogix avatar Mar 08 '20 10:03 xlogix

@xlogix Actually, I had changed the location of these files in this PR. So this location does'nt exist on my branch and so no conflicts are there on my forked repo. I'm a bit confused with this ?

vngarg avatar Mar 08 '20 10:03 vngarg

I understand that. But, right now it will show it as conflicts until you merge the master branch. Slack

xlogix avatar Mar 08 '20 10:03 xlogix

I understand that. But, right now it will show it as conflicts until you merge the master branch. Slack

@xlogix I really don't have any idea as to what should I do now ??

vngarg avatar Mar 08 '20 14:03 vngarg

I understand that. But, right now it will show it as conflicts until you merge the master branch. Slack

@xlogix sir conflicts are continuously increasing, please merge this PR now ??

vngarg avatar Mar 08 '20 17:03 vngarg

@prajwal714 Since you created the issue, could you explain why this approach would be better for us?

D3vd avatar Mar 09 '20 01:03 D3vd

Hey @vngarg. Did you perform tests for all the files that you made changes to? I am getting this error while creating a path. Screenshot from 2020-03-09 07-11-45

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.

Sorry, some of the tests might have been left untested .... I'll test all of them once again ... Again sorry for my mistake ..

vngarg avatar Mar 09 '20 03:03 vngarg

@prajwal714 Since you created the issue, could you explain why this approach would be better for us?

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.

prajwal714 avatar Mar 09 '20 07:03 prajwal714

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.

D3vd avatar Mar 10 '20 10:03 D3vd

I agree with @R3l3ntl3ss on keeping components based on their page/function. This helps to navigate the project easily.

xlogix avatar Mar 10 '20 11:03 xlogix

@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 avatar Mar 10 '20 11:03 xlogix

@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 .... But Actually I don't have an idea what you all are talking ... And as this PR has became too complicated so I too am in favour of your decision ... But please explain me that what I have to do if we proceed according to you

vngarg avatar Mar 10 '20 16:03 vngarg