Skip to content
OverviewSolutions
4
Comments
7
rikvandersar
@RikvanderSar

All comments

  • rikvandersar•145
    @RikvanderSarPosted about 3 years ago

    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
    0
  • Carl Wicker•1,055
    @carlwickerSubmitted about 3 years ago

    Any feedback welcome.

    1: I'm not sure why the validation doesn't like "all: unset".

    2: Couldn't find a good solution for coloring the SVG social media icons on hover.

    3: The generate screenshot function in FrontEndMentor doesn't render the patterns on my backgrounds although they work fine.

    ReactJS: Insure Landing Page

    1
    rikvandersar•145
    @RikvanderSarPosted about 3 years ago

    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.
    2
  • rikvandersar•145
    @RikvanderSarPosted about 3 years ago

    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
    0
  • Atul Yadav•135
    @titancode25Submitted about 3 years ago

    Hey,

    This was really awesome project as I was learning Grid CSS, Simple not much issues in this! Hope you love this content Developed by me!

    Simple! Grid

    1
    rikvandersar•145
    @RikvanderSarPosted about 3 years ago

    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.

    0
  • rikvandersar•145
    @RikvanderSarPosted over 3 years ago

    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!

    0
  • rikvandersar•145
    @RikvanderSarPosted over 3 years ago

    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
    0
  • rikvandersar•145
    @RikvanderSarPosted over 3 years ago

    Hi Adebayo,

    Look good to me! If I compare your version with the design it looks like the inactive and active state of the links are vice versa. And I wonder why you've used a psuedo element for the background image. Wouldn't a background image on the body work as well and be less code?

    Marked as helpful
    1