Skip to content
OverviewSolutions
0
Comments
2
Gin
@callmegin

All comments

  • Alfie Mitchell•190
    @alfiemitchell123Submitted about 1 year ago

    After spending a few weeks on this, I'm proud of how it turned out! I learned a lot about using React and Tailwind in this project and using a mobile-first workflow.

    My main struggle with this project was browser responsiveness with SVGs and the blue shapes. I could style the layout to look how I wanted it to at each breakpoint, but the SVGs and the blue shapes don't look right when the viewport width is between those breakpoints. What are the best practices for designing with every viewport width in mind?

    Responsive bookmark manager page using React and Tailwind CSS

    1
    Gin•20
    @callmeginPosted about 1 year ago

    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
    1
  • Gin•20
    @callmeginPosted about 1 year ago

    According to your github, you're aspiring to be come a dev. So I have one recommendation for ya:

    Code readability is key ! While everything works, but at least for me readability could be improved. And believe it or not, but writing a complex code might not impress anyone and even have an opposite effect during your code reviews :)

    You've got a bunch of states, which are maybe not really necessary and that comes down to you choosing to generate buttons from an array. Which forces you to create another state to track your button clicks and so on.

    What could improve that, in my eyes, is creating array of objects, from which you could easily generate buttons and keep track of their changes at the same time. Something like this:

    const [buttonData, setButtonData] = useState(
{id: 1, selected: false},
{id: 2, selected: false},
{id: 3, selected: false},
{id: 4, selected: false},
{id: 5, selected: false}
)

    Having that, you can generate your buttons:

    buttonData.map(button => <Button key={button.id} rating={button.rating} isClicked={selected} onClick={() => ...} />

    This way you'd get rid of some of your states, and simplify your handleButtonClick function.

    On top of that, isRated is not really necessary, since you're already keeping track of current rating, hence you can just simply check if rating is not 0 and that's it.

    That's my 2 cents :)

    0