Skip to content
OverviewSolutions
9
Comments
4
Rob Vermeer
@RobVermeer

All comments

  • Rob Vermeer•170
    @RobVermeerPosted over 2 years ago

    Solution looks good!

    One best practise I would recommend, is to always add the width and height of the image to the img element. These should be the full size of the image (intrinsic size). Without these values the browser doesn't know what amount of space to reserve when downloading the image.

    You can see this in action in for example Chrome, by opening the element inspector, go to network and disable cache and throttle with "Slow 3G". Then go to performance tab and reload, when page is full loaded, stop the measurement and hover over the screenshots to see the text moving from top to bottom when image loads in (you see a peak in the graph where is happens) =)

    You can read more about this here https://web.dev/cls/ and https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img

    Marked as helpful
    1
  • Rob Vermeer•170
    @RobVermeerPosted over 2 years ago

    Looking good Adarsh! Love the smooth animation =)

    Some feedback about the semantic meaning of your HTML, I think it's better to use a form as wrapper for the component and use radio buttons for the different rating options. This way you can have a submit button that submits the form, which will result in simpler JS (only a submit event listener is needed).

    A bonus is that you then can make the radio group required, so the browser can do the validation for you.

    This also makes the component accessible for screen-readers and for users navigating with keyboard =)

    Marked as helpful
    2
  • Rob Vermeer•170
    @RobVermeerPosted over 2 years ago

    Nice solution!

    I only have one small but in my opinion important point of feedback. Images should always have a width and height defined on the element. This way the intrinsic size of the image is known to the browser and the browser already knows how much space it needs to reserve for it. Without this you would see a layout shift whenever the image is loaded in (this is especially visible when user has slow internet). Nowadays Google thinks this is really important =)

    You can read more about this here https://web.dev/cls/ and https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img

    Marked as helpful
    0
  • Rob Vermeer•170
    @RobVermeerPosted over 2 years ago

    It looks really good! Like that you made the form and thank you screen the same size.

    Some feedback on the code: you should use a bit more semantic elements. Since this looks like a form, let's use a form element. Also for the options a radio group would make the most sense to me. This way you can also simplify your JS, so that all the logic would live in the form submit event listener.

    Keep it up!

    Marked as helpful
    1