Full Stack project review

Here's a short review of your Full Stack course project. The comments in the review are suggestions that you can use in this project or in your future projects. None of the suggestions are required to get a passing grade.

User experience

What did I do??

Played around in the first page I clicked around on every page Watched a video partially

Experience

The app is really clean looking and feels professional.

See https://portfolio.johnlee012.com/works/software, at least in my browser (square screen) the text overlaps with the images. Both being white makes the title almost completely unreadable.

Othewise the experience is excellent. Only thing I'd go for is optimizing the first load with lazy loads for components you don't need to get started with rendering: https://reactjs.org/docs/code-splitting.html

Code

I saw you use foldered components for project and worksCard but not for others. Both work well for a project of this size. You could also do them page-by-page basis with some common components.

I was expecting the images to be in the public folder.

There is a large amount of magic numbers. Looking at global.js here..

In addition styles are all over the place, in css, in js files and inlined with the components. I'm personally more a scss guy but the way you're trying to do them looks close to this: https://cssinjs.org/?v=v10.5.1 Maybe quick refactor with the help of this project?

I'm not completely certain why https://github.com/delphi1004/portfolio_v2/blob/893fa993ce06a4fdbabd7c535c020aa617d96f4e/src/reducer/statusReducer.js#L15-L24 instead of

export const setCurrentMenu = (id) => ({
  type: "SET_CURRENT_MENU",
  currentMenu: id,
});

but that might just be me. You can comment if there's something I'm not seeing here.

Don't do this: https://github.com/delphi1004/portfolio_v2/blob/893fa993ce06a4fdbabd7c535c020aa617d96f4e/src/components/projectBase.jsx#L34-L35 Use https://reactrouter.com/web/guides/quick-start instead. See the "Topic" on the quick-start page and how you could access the params from the props.

The code is well written and split into logical parts.

Conclusion

Funnily enough a full stack project with only half a stack. Absolutely beautiful project. Well done!

Hi Jami,

I really appreciate your professional and practical comments on my project. I modified my project based on your comments and please find my modifications below.

Experience

See https://portfolio.johnlee012.com/works/software, at least in my browser (square screen) the text overlaps with the images. Both being white makes the title almost completely unreadable.

-> I understand what you mean and it is part of my design concept. when you hover your mouse on the images then the title moves to the right side. It is a kind of fun interaction I just wanted to add and better looking for the text alignment. My intention was to move the text when the user tries to slide the images in the image gallery.

https://user-images.githubusercontent.com/29729220/108169272-d6760500-713b-11eb-9a1d-858c1747f0ed.mp4

-> Right, slow loading was the BIG question of my project. thanks for letting me know about lazy loading. I'll study and implement it for the next commit.

Code

I saw you use foldered components for project and worksCard but not for others. Both work well for a project of this size. You could also do them page-by-page basis with some common components.

-> Yes, I made directories for the each pages.

I was expecting the images to be in the public folder.

-> Yes,I moved it to the public folder.

There is a large amount of magic numbers. Looking at global.js here.

->Right, most of the numbers are related to the aspect ratio and the image scale for the image gallery component I made. I tried to not using the magic numbers. There must be a nicer solution, however, I couldn't find a better responsive solution for resizing the image when the user changes the browser size.

-> I'll try to use the scss you mensioned for the next commit.

I'm not completely certain why https://github.com/delphi1004/portfolio_v2/blob/893fa993ce06a4fdbabd7c535c020aa617d96f4e/src/reducer/statusReducer.js#L15-L24 instead of

export const setCurrentMenu = (id) => ({
  type: "SET_CURRENT_MENU",
  currentMenu: id,
});

but that might just be me. You can comment if there's something I'm not seeing here.

-> You are right, it was redundant. I modified the code followed by your comment.

-> Yes, I modified to use 'useParams'. It saves several lines of code and looks much cleaner.

Once again thank you so much for your time, I'll try to solve lazy loading and styles issues for the next commit. I may need some time to study and try these solutions.

delphi1004 / portfolio_v2