keithb418 / scrum-team-app

Apache License 2.0
3 stars 5 forks source link

Cleanup #108

Closed Crunchwrap78 closed 6 years ago

Crunchwrap78 commented 6 years ago

Things to note.

Crunchwrap78 commented 6 years ago

@keithb418 1) will fix 2) I addressed this in the comments 3) will fix

other questions 1) I thought I posted that comment but I guess not. react-router-redux is unnecessary in react-router-dom since it internally handles your history. if you were using the older version before version 4 then yes you would need it. 2) with redux your store is never persisted so you always have to reload your data. There are other remedies to solve this in react like graphql + apollo or relay to bind the data to components. This will speed up performance in loading our app from different views since our props are always persisted. I will note I'm not a complete expert at this seeing as my state structure is pretty basic, but I understand the logistics of it. In general we should only use the data needed in our components, but I added everything in state since in the future the structure will change. I haven't seen any articles that have said you shouldn't do this and actually some encouraged it. You can actually view twitter's state in the local storage

asc2683 commented 6 years ago

This is more of a general comment to do with project branch naming conventions. Since this is more of a major refactoring task, perhaps consider naming the branch as refactor/release-1.1

See contributing doc for branch naming convention: https://github.com/keithb418/scrum-team-app/blob/development/CONTRIBUTING.md

Also, for future refactoring, breaking the refactoring to small tasks into specific modules or functionality (i.e., webpact, react-router, or redux state manegement) would be helpful.

Crunchwrap78 commented 6 years ago

@asc2683 The react-router and webpack refactor went hand in hand with the dynamic imports. The only thing that should have been separated was the redux state management. After this merges I'll get started on unit tests