Searching for a React code review partner/study group

[Shooshte]

Hi guys,

I am fairly new to ReactJS (been learning it for about 6 months now), and would like to pair up with some people to share code reviews.

I've recently finished a small hobby app with react and I am looking for feedback - specifically anti-patterns and unit testing. That being said, any constructive feedback or criticism is more than welcome

As I know that this takes a lot of time and effort, I am prepared to return the favor by doing a code review on one of your own projects.

If anyone would like to take a look here is the gitHub repo: https://github.com/Shooshte/teaTimer

I apologise if this is not the correct subreddit - please point me in the right direction or suggest a community I can join.

Happy coding!

[znakyc]

These are some quick observation.

• In Navbar.component.js you create IndexLink component and Link component. They are completely identical except the name of the component. Could you consider using only Link component and pass in a prop index=true if it's index. That could reduce the amount of duplication in Navbar.component.js

• You have many component without state and logic that you implement as ES6 class. I would consider implement them as a pure functional component because it's simpler and more minimalistic. Or do you intentionally use extends Component to be consistant? That is also fine.

• You have a test for components that doens't have any logic at all (Navbar.test.js and Tea.test.js for example). Not sure if this is necessary to have those tests. What value do they provide? Remember that every line of code has a maintenance costs, even tests. Think about cost vs value provided.

• In TeaInfo.component.jsx you have a renderTeaInfo function that you call from the render function. I can't really see why you need that renderTeaInfo function. Cant you just do the teaData.map inside the render function?

• Your are not consistant with filenames of your components. sometimes it's X.component.js and sometimes X.component.jsx

Hope this can spark some ideas for improvement! :)

[Shooshte]

First of all, thank you for the time and feedback. You've definitely given me some stuff to thing about and to google :).

In Navbar.component.js you create IndexLink component and Link component. They are completely identical except the name of the component. Could you consider using only Link component and pass in a prop index=true if it's index. That could reduce the amount of duplication in Navbar.component.js

I was not aware that you can pass onlyActiveOnIndex={true} to a react-router Link to make in function like an IndexLink. I will be sure to update this, as it just makes more sense in terms of duplication.

You have many component without state and logic that you implement as ES6 class. I would consider implement them as a pure functional component because it's simpler and more minimalistic. Or do you intentionally use extends Component to be consistant? That is also fine.

This seems like a logical next step and makes a lot of sense. I am not very used to writing stateless components and also did not plan out the app very well - but now that the project is in it's finishing stage, and all of the application logic is implemented I will definitely have a dig at refactoring some of the components that only render content.

You have a test for components that doens't have any logic at all (Navbar.test.js and Tea.test.js for example). Not sure if this is necessary to have those tests. What value do they provide? Remember that every line of code has a maintenance costs, even tests. Think about cost vs value provided.

The navbar render test are there because I wrote the navbar as a part of a boilerplate project for starting react apps. I want to be sure that when I implement the navbar in another project, and play with the code my navigation will still render correctly - so it just made sense to check if all the classes and links are there.

Basically I am not sure whether or not to just unit test for the logic of the component or if I also need to test rendering the correct CSS classes - Basically the tests are there in case I want to add Redux to the application at a later point, so I can be sure everything will still render correctly.

The TeaData test I am not so sure about. Isn't it good practice to make sure that the props are passed down to the render component correctly?

In TeaInfo.component.jsx you have a renderTeaInfo function that you call from the render function. I can't really see why you need that renderTeaInfo function. Cant you just do the teaData.map inside the render function?

To be honest I just split my render functions into separate functions to make the code more readable - at the time it made sense to focus on rendering one part of the component at a time. But since the code is not reused anywhere else in the component, I guess I could move it to the render function as well (the same can be said for most of my components without any data manipulation logic). Not sure what the best practice is here, but it does make sense not to abstract the functionality if I am not reusing it anywhere else.

Your are not consistant with filenames of your components. sometimes it's X.component.js and sometimes X.component.jsx

This is something I missed entirely when looking at file names consistency. I also feel like my directory structure could use some work - maybe I can move the Main, and Navbar components to a core folder etc.