Responsive Design Portfolio with a Carousel

Community feedback

• 

  Aman Singh Bhogal•1,010

  @asbhogalPosted over 1 year ago

  Hi Bogdan,

  This is a great solution - well done! You've used grid well here, as documented, as well as rem values and a reset, and its responsive. Nice. Just a few things I thought I'd mention which is more from a management/maintainability perspective:

  • With the Skills.jsx, to avoid repeated code, you can declare an array of objects with key-value pairs for the icon, text and classname (instead of using them as props) then map() over this array and render a <Card /> component for each, passing in the array index for the key prop and the respective three properties.
  • You've separated your variables, reset and utility classes, but I'd also recommend splitting your App.scss into individual components for easier management and debugging - it reduces cognitive load, as isolating a single element from a long stylesheet can take some time. You can search for it, but I t think its better to approach component styling the way you approach component logic and split them.
  • Locally host your Google Fonts for privacy and performance reasons. You can use the fontsource npm package to install the typefaces you need and then import the weights required, then declare the font-family where required. Here's their documentation. Link. I use this a lot and find it incredibly easy to work with. They install in the web-recommended woff2 format and you only end up with what you've used in your production build, and you can import them into config files for UI Component libraries too, making it very versatile.
  • In terms of SEO you're missing the meta description for crawlers to index your site page. While this is just a project, it's good practice to get into from the start so it becomes natural

  Also, from a personal perspective, I think using either styled components or SASS in a project is best, instead of combining them. While there isn't a hard rule for this and some would argue against it, having one approach (either coupled for single management or separated concerns) means it'll be easier to locate and debug styling and possible prevent code clashes. That's just my two cents.

  Hope this helps!

  Marked as helpful

  1

  ---

  

  Bogdan Kim•780

  @semperprimumPosted over 1 year ago

  @asbhogal Wow, Aman, thank you so much for the feedback! This is probably the first really meaningful feedback I ever got here!

  • Yes, I should've created an array of objects for a Skills.jsx grid. I did that for a carousel, but I forgot about the skills grid. I'll refactor my code a little later :)
  • Thanks for the advice on splitting the App.scss! I'm just getting used to using React. I actually thought about what I can do with it because the CSS file was getting a little annoying as it grew. Noted!
  • I will try the fontsource package in my next project :)
  • Yeah, I really should do my research on the meta descriptions.

  I see your point; I should try using one thing at a time. I was trying to use styled-components more, but I couldn't get it to work with Swiper; it broke everything in production. I think this was the moment when I said, "Screw it," and decided to use SCSS for the rest of the project. I'll try using styled components mostly next time

  Hey, thank you so much for the feedback again; it's very helpful!

  1

  

  Aman Singh Bhogal•1,010

  @asbhogalPosted over 1 year ago

  @semperprimum no problem at all, glad to help!

  1