Skip to content
OverviewSolutions
4
Comments
9

Jason Heys

@heyitsganySydney, Australia175 points

Freelance front-end web developer, living in Sydney with his wife and dog.

I’m currently learning...

React

Latest solutions

Latest comments

  • Jason Heys•175
    @heyitsgany

    You've managed to recreate the design pretty faithfully, and your responsiveness works. Nice job!

    However, there are a few things you need to look at:

    • Your media queries keep the mobile design until the screen is at 1800px wide. This is contrary to the design file, which I'm sure gives a width of 1440px for desktop. (This is why your design comparison screenshot looks wrong above).
    • Your class names could use some work. While there is no rule against using camelCase when writing class names, there is a convention of using lower case seperated by hypens. Although I would recommend reading up on BEM naming for class names.
    • You want to make sure you're using semantic HTML elements when possible, instead of just using <div>. This goes a long way in improving site accessibility for things such as screen readers. (You want to at least throw your mainContainer class into a <main> element instead of the <div>).
    • There are a couple of things in your CSS that could be changed:
      • To make sure you're keeping your code DRY, I'd suggest setting the font-family on the body instead of setting it in two different classes. This makes it easier for the font for the page to be changed (changing one line of code instead of multiple).
      • Your flexBoxContainer class has a set of CSS properties that don't do anything to the style until you add a display:flex to the class in a media query. I'd suggest grouping this all together in the media query.
      • You set a background on the HTML. While this technically works, you should really apply this styling to the body and not html.
    • You're using React for a single page site with no (or very minimal) interactivity. This site would be a better use case for using just HTML and CSS. The design as it stands doesn't indicate the need for any JavaScript and I feel like React maybe overcomplicates the process here.

    That's just a few of the things I could see while looking. Hopefully these give you some things to work on and help you learn in the process. You've done a great job, and you should be pleased with what you produced! Keep it up!

    Marked as helpful
  • Jason Heys•175
    @heyitsgany

    This looks good, you've gotten pretty close to the design, nice work!

    A couple of things to look at:

    • Your active state on the image (the colour overlay and eye icon) is a little larger than the image itself, so you get an odd extra bit at the bottom of the image when you hover.
    • You want to change the attribution div to a footer element instead (just to keep it semantically correct).
    • The height of the card could be increased slightly to better match the design.

    Also, the solution report is stating that you need to include a h1 element, although looking at your markup I can see it's there. I'm not sure if making it aria-hidden has caused this issue?

    Marked as helpful
  • Jason Heys•175
    @heyitsgany

    I think it switching to the column layout below 1440px is an odd choice. You want to find the resolution where the horizontal layout starts looking cramped or broken and then switch to the vertical layout.

    You'll want to think about adding a h1 tag somewhere, as semantically a page must contain a level one heading. As the design doesn't have a good place for it, I'd suggest adding a h1 tag with screen reader only styling (so it's only visible to screen readers), using the title of the challenge.

    Also, if you want to get your positioning closer to the design I'd suggest making sure your min-height on your body is 100vh, then giving it a display: grid and place-items: center.

    Otherwise, this is great; the styling on the card pretty much matches the design. Nice work!

  • Jason Heys•175
    @heyitsgany

    Looks good. Well done on getting it close to the design.

    • Couple of things to work on, you haven't implemented the hover/active states for the buttons and links (change, proceed and cancel). That's a pretty simple thing to add.
    • Try to move the background image from your HTML to your CSS, using the background property. This way you have greater control over how it displays. (Also tweak your background colours as they're off a little).
    • Make sure you're using semantic HTML (div and span are non-semantic), make use of elements like header, footer and main. As your card is the main content of the page, change it from <div class="order-card"> to <main class="order-card">.
    • Finally, (and this is a preference thing) you could remove the box-sizing from your html and just use it on your reset.
    *,
*::after,
*::before {
    margin: 0;
    padding: 0;
    box-sizing: border-box;
}
    Marked as helpful
  • Jason Heys•175
    @heyitsgany

    Hey, nice job. You've got nice and close to the design and it looks good!

    There are a couple of things that I've noticed that you could work on to improve it.

    • Firstly, you give your body an ID? This doesn't seem necessary as we can already style the body using body.
    • You can apply the background "wave" image directly to the body in the CSS. This allows you to remove an unnecessary div. (Along with the z-index: -10).
    • Instead of using aria roles, why not just use the elements themselves? The whole thing can be wrapped in a <main> element. Your footer can just as easily sit in a <footer> element.
    • Don't forget to change the cursor when you hover the button and link elements. (Using "cursor: pointer").
    Marked as helpful
  • Jason Heys•175
    @heyitsgany

    As you've already mentioned, this website is not responsive. You'll need to use media queries to set the styling for mobile devices.

    These are a few things to look into fixing with your design (although certainly not an exhaustive list).

    • Make sure you're using landmark elements (such as main, footer, nav, section) instead of always using a div. (You have used these a class declarations, so you know where they need to be).
    • Instead of pasting the code from the SVG images into the HTML, you can of course use an <img> tag to place the image. This should make your HTML more readable (however it doesn't break things if you don't do this).
    • Following on from this, you have placed the background image into the HTML, instead of using CSS to place the background. My advice would be to set the background on the body using the background CSS property.
    • Your CSS has very strict specificity. You normally don't need to use so many selectors to style an element. For example, you use ".container .card .body .about .details", where you could achieve the same styling just using the ".details" selector.

    Keep working on it, and you'll get there!

View more comments