search

npvisual / ToDoList-Redux

Redux based ToDo / Task application
4 stars 2 forks source link

Sharing some ideas #1

Closed luizmb closed 4 years ago

luizmb commented 4 years ago

Hey, nice job!!!

I'm sharing some ideas, really nitpicking a lot not necessarily because there's a right or wrong way of doing these things, but to expose a different perspective about a large variety of things that will allow you to pick the ideas that work better for you, in each scenario. Some of these things will probably be useful as you grow the app and add more functionality. Others are purely preference style.

I'm gonna put some inline comments to explain and justify the change, feel free to add questions on those and, once this PR is clear, please feel free to delete it together with the branch.

npvisual commented 4 years ago

🎉 Awesome ! Thanks for the feedback. I will go over all the comments. Since I was trying to use the ForEach extension as my next step today, this will play well into that refactor I believe.

Oh, btw, I am (obviously) able to use CombineRextensions now.

luizmb commented 4 years ago

Pick the ones you like, discard the ones you don't like. I tried to give a completely different perspective as much as I could, to offer more alternatives for you. Now it's up to you to pick what you like. Then you can discard this PR.

npvisual commented 4 years ago

I opted to merge your pull request given the amount of great information / comment there. Will follow up with an additional PR on some more "cosmetic" changes.