search

firasrg / spring-petclinic-reactjs

Modern ReactJS client for Spring Petclinic sample application
0 stars 0 forks source link

[Refactor] Add Vets #4

Closed michaelisvy closed 1 year ago

nilshartmann commented 1 year ago

Hi @michaelisvy,

great work! I added some commments to your changes, hope that's ok!

firasrg commented 1 year ago

Recap Review Feedback:

@michaelisvy I've completed my review and would like to provide some general feedback.

Code Organization

It appears that the code could benefit from better organization and structure. For example, the use of hooks and functions. I encourage you to put a JSDoc comments to help us understand quickly your updates. Modularity helps developers to read the codebase and the logic fast ⚡️.

Commit History

I noticed that there are 5 commits for 9 files. While it's not inherently bad to have multiple commits, it would be beneficial to group related changes together, at the end, before creating the PR. This helps provide a more concise overview of the changes. Consider squashing some commits that address similar functionality or changes please.

Typing

Please try to focus on typing by providing types/interfaces for pure functions, components, hooks calls or anything that can be typed. Typing helps to avoid mysterious unexplained issues, regressions or bugs. I suggest to look at React TypeScript Cheatsheets to learn more about typing in React.

Conclusion

Remember, we strive for a well-structured and maintainable codebase, so taking these considerations into account will contribute to the overall quality of the project. Also, make sure you're following the lint configurations.

Feel free to reach out if you have any questions or require further clarification on any of the points mentioned above.

Best regards,

arey commented 1 year ago

Hi @nilshartmann and @firasrg Thanks for all your feedback to this PR. Can this PR be merged so that Michael can continue on other issues?

firasrg commented 1 year ago

Hello @arey !

Sorry for the delay, I was absolutely off cuz of some holidays 😅.

I think this PR still needs to be reviewed but as it took enough time waiting. I think to merge it, but will consider the rest of the review points in upcoming issues.

Thanks

arey commented 1 year ago

Thanks for the answer @firasrg and welcome back from holidays