@callmegin
Posted
Hey there ! A couple of things.
Most importantly though, don't forget to setup .gitignore
!!! node_modules
takes insane amounts of space and on top of that, they should never be in your repo !
Noticed a couple of components IllustrationFeaturesTab*.js
with SVGs which are wrapped in styled div
. That looks quite a bit messy. I'd rather separate all of my SVG icons and import them into component where I'd have styled div
container for them.
Or even better, have a separate container component which is dedicated for SVG icons (that component could wait for children elements or something along those lines).
But since you're using NextJs, look into svgr
, then you'd simply import SVG like so:
import MySVG from '../assets/logo.svg
and use it as a component
import MySVG from '../assets/logo.svg
...
return (
<div>
<MySvg aria-label='logo'/>
</div>
This would allow you to get rid of your *.js
files which are actually SVGs. And for me - that's the preferable way.
Next - you could reduce your boilerplate in FeaturesTab.js
, you've got a lot of li
elements, which literally do the same thing and has the same core styling, by creating a reusable component, you'd have a nice and clean code and on top of that - get some practice on code splitting a little bit.
EmailSignup.js
by the looks of it, users would always get Whoops, make sure it's an email
notification :)
Welp, that's a quick code review, there are some other stuff that could be improved, but it's a personal project and not work :D
Marked as helpful
@alfiemitchell123
Posted
@callmegin
Hi Gin,
Thank you for the constructive advice! The organisation of the SVGs was another issue I was having, as the file management was getting fairly messy, but I wasn't sure how to get around that. I appreciate the pointers, and I'll look into everything that you've mentioned.
Thanks, Alfie
@callmegin
Posted
@alfiemitchell123
No worries mate, if you get stuck or anything, feel free to ask more questions here, I'll try to help you out