@steventoben
Posted
For code quality I'd highly recommend code splitting. You're functional components are really large, generally a component should be broken down to the deepest level. It looks like you should break down Nav into it's own component, List, as well as list items. You should also use the mapping feature to map list items to a list so you don't have to repeat lines of code. It looks like you're using anchor elements and images enough to break them into their own component. On your hamburger menu the toggling state is overly complicated, as well as bad practice because state is async, so it could not be updated yet when that function is called. The best way to prevent that is just in your function writing setActive((prevState) => {return !prevState}); I'm pretty sure your useEffect hook is wrecking havoc on your performance, you have no dependency array so it's firing every single render, and useEffect is for managing side effects, so setting up the event listener to observe the window size is fine, but you must write a return statement that removes the event listener to clean up the effects. also add a dependency array to the hook containing window.innerWidth, that way it will only fire if that value changes. You could also consider debouncing since you really only want the value at the end of a resize. Also, you're not using props at all? props is one of the most important features in react and should be heavily incorporated in the app composition, instead of manually writing all of your data. Also one last thing, try to avoid styling like you did in the Creations functions. It looks nice but it hits performance heavy, especially an onMouseMove call, react will reconstruct all of the functions and variables at each render unless memoized or placed in a ref, so grabbing directly from the DOM over and over and repainting will cause some performance issues usually.
Onto the styles.... we've already talked too much about font-size lmao so let's just ignore that. In general your sass is pretty dirty. I'd recommend trying to compile your sass and take a look at the output of the css file, that would probably explain much better than I could. Your nesting is far too deep and often times unnecessary. Try to practice DRY and create a mixin for commonly used styles. If you have sass that's more than 3 levels deep there is something wrong with your method. Also it common practice to put media queries inside of selectors, not the other way around. As well as using em units for the media queries, also line-height should be unit-less, not measured in rem.
React and sass both have an atomic kind of methodology, so break down components as much as possible, use props and refs, it's ok to have hundreds of React components, and is actually quite common and really shows the beauty of React when you atomize everything.
Overall tho it looks good, you could try using css modules if you want to try a different method of styling, they're scoped to each component so you don't need to worry about clashing class names.
@ApplePieGiraffe
Posted
@steventoben
Wow, thanks. I really needed advice for React! I knew the logic for the mobile navigation menu was pretty bad and my Sass was quite ugly, but I didn't bother fixing those things (until now, I guess). 😀
To be clear, should I split my components into just separate functions or simply create new files for each component?
@steventoben
Posted
@ApplePieGiraffe No problem! If you need any React help I'm probably a good resource since I've been using it professionally for like 3 years so I have a pretty good grasp on most of it. It really depends on the situation, for example on this project here I'd make a Nav.jsx and a List.jsx but in that List component I'd make a ListItem function. If you did that you could also set List.Item = ListItem; Then export the List. Then when writing a list it would look like <List> <List.Item> List Item 1 </List.Item> </List> and that would let you write some cleaner code and keep both List and ListItem functions in the List file. It's ok to have multiple components in a file as long as they're tightly coupled, basically like helper components that wouldn't be used without the main component. Just like you'd never use an <li> element without a <ul>.
And yeah it's ok to use the nesting feature and it's an amazing feature but people often go overboard and don't think about what that sass code will look like as css code, cause in the end it's going to be converted to css so nesting will create tons of code repetition. It's a great feature but just make sure to think about what the css compilation will look like.
@ApplePieGiraffe
Posted
@steventoben
Okay, cool! 👍
