Jason Heys

Sydney, Australia

Aspiring web developer, living in Sydney with his wife and dog. Learning HTML, CSS and JavaScript.

All Comments

    • HTML
    • CSS

    First Challenge - Using Flexbox

    Jason Heys115 | Posted about 10 hours agocommented on Pablo Clemente's "Order summary component" solution

    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.
    *::before {
        margin: 0;
        padding: 0;
        box-sizing: border-box;
    • HTML
    • CSS

    Order Summary challenge solution

    Jason Heys115 | Posted 20 days agocommented on Jarek's "Order summary component" solution

    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").
    • HTML
    • CSS

    Not Responsive

    Jason Heys115 | Posted about 1 month agocommented on Ibraheem's "Order summary component" solution

    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!

    • HTML
    • CSS

    HTMl5 and CSS3(flexbox and media queries)

    Jason Heys115 | Posted about 1 month agocommented on Vinit Kumbhare's "Order summary component" solution

    Just from a quick look at the live site and the code, these are a few things I've noticed.

    • You missed the point in the style guide on the font-family. This design wants you to use "Red Hat Display" from Google Fonts. It doesn't break anything, but it's a quick way your site closer to the design.
    • Your container class could quite easily be removed and the styling placed on the body. Setting a min-height of 100vh would ensure the background image covers the entire screen, instead of having blank space at the bottom.
    • You have a lot of "box" divs in your html, box-3 seems to serve some purpose to group the music notes image, the annual plan text and the change button. You could probably lose most of the box classes and just style the elements. Especially as you're using flex and that should keep things looking tidy.
    • You set the border radius for the card in two separate places, first the top corners on the design class, and then the bottom two on the summary class. As these are all nested in a "card" class, you could just use border-radius and add an overflow: hidden to achieve the same effect.
    • There is also at least one instance where you overwrite a style in the same selector.
    • HTML
    • CSS


    Jason Heys115 | Posted about 1 month agocommented on Alice's "Order summary component" solution

    Looks good! You've followed the design well. Just a couple of things to work on. Firstly, you haven't implemented the hover effects to the buttons and anchor tag.

    Secondly, to help with accessibility (for screen readers) change the container div element to a main element instead.

    Also, you haven't implemented the responsive design for mobile. So, your design touches the sides of the viewport at resolutions lower than 375px.

    As for your card moving around on your background. I'm not sure what you mean.

    • HTML
    • CSS

    Music Card

    Jason Heys115 | Posted about 1 month agocommented on Jontim's "Order summary component" solution

    Nicely done! Your solution looks good, although looking at the live version on GitHub pages there are a few things that could be changed to improve it.

    Firstly, changing the 'container' div to a main element would solve the accessibility issue that your solution is getting.

    Secondly, your background image is mispositioned and doesn't repeat. So, look at making it repeat on the x-axis and let it position itself.

    Otherwise nice work!