Skip to content
OverviewSolutions
4
Comments
7

rikvandersar

@RikvanderSarNetherlands145 points

front-end developer

Latest solutions

Latest comments

  • rikvandersar•145
    @RikvanderSar

    Hey,

    Great job on this challenge!

    A couple of things I've noticed looking at your code.

    • First of there are a couple of accessibility issues reported, you should get that fixed.
    • Check your html code first and try to replace the divs for semantic html
    • In your CSS your using em units for almost everything. I think em units are particulary usefull for padding and margin in a specific section where you want the section layout to adjust according to the fontsizes in that area. For the height and width of your card or border radius (for example) it might be more logical to use px. Your could use max-width and max-height if you want to keep responsiveness.
    Marked as helpful
  • rikvandersar•145
    @RikvanderSar

    Hi Carl,

    Nice job on this challenge.

    I don't know anything about React. But I've noticed two things that might be worthwhile to look at.

    • The hero image doesn't grow larger as screensize gets bigger. This might not be mentioned in the challenge, but if I go a little wider then mobile width the hero and the footer don't grow / change with it. This might be a nice thing to fix with relative units and an extra breakpoint.
    • The other thing is when your mobile menu is opened and then somebody enlarges the screen, you're page doesn't scroll down anymore. This problably has something to do with overflow restrictions and swithing that of in a breakpoint.
  • rikvandersar•145
    @RikvanderSar

    Hi,

    Just wanted to give you some quick feedback. Great job on this challenge! Just a couple of things you might want to take a look at:

    • The div that you called 'container' might be more appropriate to call 'card'.
    • You don't really need flexbox on you container div. It is now used to center the content of your card in the middle. But you could also try to fix the challenge without flexbox on the container. If you do that, don't forget to put 'display: inline-block' on you're <a> tags end center them with margin: 0 auto.
    • On your 'plano-container' you use a margin-left: -40. It looks like this pulls the div out of the middle because you've already used flexbox, space-around on the parent.
    • I think it's a good idea to use English in all you're documents and naming so that it is easier for other people to understand it.

    Great job! Hope this feedback helps you a little bit further.

    Marked as helpful
  • rikvandersar•145
    @RikvanderSar

    Hey man. Nice job!

    Just wanted to give you some quick feedback. In the tablet / smartphone view you have some horizontal scrolling. It seems some content is overflowing. And you got some accessability issues in your markup that might need to be adressed.

  • rikvandersar•145
    @RikvanderSar

    Hi Matt,

    Thank you for this helpfull feedback, really appreciate it! I'm definitly learning a lot from this project.

    I'll get back at it with your feedback and will post a update soon!

  • rikvandersar•145
    @RikvanderSar

    If you give your body a background image and have a child element for your card the card will lay on top of your background image. But you have to set a height and width for you body to display to full image. I'd go with height: 100vh

    Marked as helpful
View more comments