ReactJS, Material UI component and code organization

Question

This is my first post here so I hope you will be patient if I'm doing something wrong. I made a small website with create-react-app and I would like to request feedback about. This website was a test and I had a limited amount of time to complete it, hence there are no tests nor license, which for the test purposes weren't necessary.

The main things I would like to have feedback on are:

1) Are my components organized properly?

2) Is the pattern I'm using to develop components appropriate?

Although I'm posting here some code, I would like to ask if you could take a quick look at my github repository and let me know what you think about it: https://github.com/sickdyd/foriio

For the components organization I used the following structure:

enter image description here

To avoid having multiple nested folders, I decided to put all of the sub components that belong to a specific component on the same level. The naming should give away the function, and the folder name has the same name as the main component.

One quick question: the component src/components/body/UserProfile/UserWorks.js is reused in src/components/body/UserWork/UserWork.js; is it ok to leave them with this structure?

The components generally have the following code structure (this is one of the simplest components):

import React from 'react';
import PropTypes from 'prop-types';
import Typography from '@material-ui/core/Typography';
import Skeleton from '@material-ui/lab/Skeleton';
import { makeStyles } from '@material-ui/styles';
const useStyles = makeStyles(theme => ({
 bio: {
 display: "flex",
 justifyContent: "center",
 marginTop: theme.spacing(2),
 color: theme.palette.text.primary,
 },
}));
export default function UserBio(props){
 const classes = useStyles();
 const {
 loading,
 profile,
 } = props;
 return (
 loading ?
 <div className={ classes.bio }>
 <Skeleton variant="rect" style={{ marginBottom: 12, width: 315 }} />
 <Skeleton variant="rect" style={{ marginBottom: 12, width: 315 }} />
 <Skeleton variant="rect" style={{ marginBottom: 12, width: 315 }} />
 </div>
 :
 <div className={ classes.bio }>
 <Typography>
 { profile.bio }
 </Typography>
 </div>
 )
}
UserBio.propTypes = {
 loading: PropTypes.bool.isRequired,
 profile: PropTypes.object,
}

• IEatBagels: If you'd like to add more code from your repository to your post, don't hesitate :) Anyways, I think your first post is good, welcome to CR!

Answer 1

Your file structure is okay, though it is not the "conventional" react folder structure. I personally like your approach better than having lots of nested folders with index.js but this is because my workflow relies on searching files by name, and searching for index.js is impossible — you always have to search by file.

One quick question: the component src/components/body/UserProfile/UserWorks.js is reused in src/components/body/UserWork/UserWork.js; is it ok to leave them with this structure?

As I understand, UserWorks is some component that shows some sort of preview of works; I think that the structure should be the other way around, UserProfile should depend on UserWorks not the other way around. You might even structure it explicitly that UserProfile and UserWork pages (components) depend on a representation component UserWorks, though naming becomes a bit cumbersome.

Note on using export default

It seems to be standard "good" practice to use export default in Javascript, but I personally don't like it for various reasons and one of them shows in UserWork: you are importing UserWorks as UserRelatedWorks this suggests that UserWorks is used with different intention that is originally designed. Named imports would not eliminate this issue completely but would at least force explicit statement import {UserWorks as UserRelatedWorks} from ...

Comments on the code:

1. lots of unnecessary empty lines, this is a bit unusual. In most cases empty lines are used to group statements together, here they appear to be random and make code harder to read.

2. you are using classes for div.bio but not for Skeleton though those styles are repeated but bio is not (or should not be)

3. no matter value of loading you are returning a container with <div className={classes.bio}>, you can return container and resolve content inside of it based on the loading to avoid duplication.

---

In this code filter method is used incorrectly, the callback should return boolean but in this case returns the object itself or null, this relies on implicit type casting to truthy value and is very confusing

 works.filter(w => {
 if ((!category) || (category === "all")) {
 return w;
 }
 else {
 if (w.category_list.includes(category)) return w;
 }
 return null;
 })

this code should be

works.filter(work => !category || category === 'all' || w.category_list.includes(category))

• devamat: Thank you for your feedback! I edited the code. I usually add white lines because it helps me read through the code, but I see your point. If the standard is to not have them I will clear them out and make the code more compact.