Skip to content
Submitted almost 2 years ago

Product preview card component

Swing95•180
@Swing95
Preview site (opens in new tab)View code (opens in new tab)
This is a solution for...Product preview card component
  • HTML
  • CSS
1newbie

Design comparison

SolutionDesign

Community feedback

  • Melvin Aguilar 🧑🏻‍💻•61,140
    @MelvinAguilarPosted almost 2 years ago

    Hello there 👋. Good job on completing the challenge !

    I have some suggestions about your code that might interest you.

    HTML 📄:

    • Use the <main> tag to wrap all the main content of the page instead of the <div> tag. With this semantic element you can improve the accessibility of your page.
    • The <h1> is the most important heading on the page, In this challenge the perfumer's name can be considered like the title of the page, so it should be the <h1>

    • The <br> tag is not a semantic element. If a screen reader user is reading the page, they will hear "line break", which breaks the flow of the content. Instead, use CSS properties like margin and padding to add vertical space between elements.

    • The alt attribute is used to provide a text description of the image which is useful for screen reader users, assistive technology users, and search engine optimization. Add the alt attribute to the <img> tag of the product.

    CSS 🎨:

    • Setting width on the html tag is not recommended as it can cause issues with responsiveness and cross-browser compatibility. When you set a fixed width to the <html> element, it will only work well when the viewport has the same width, this means that the website will not adapt to the different screen sizes, and will look bad when it is accessed on a smaller device.
    • The width: 100% property in the body tag is not necessary. The body tag is a block element and it will take the full width of the page by default.
    • Setting the width of the component with a percentage or a viewport unit will behave strangely on mobile devices or large screens. You should use a max-width of 900px to make sure that the component will have a maximum width of 900px on any device, also remove the width property with a percentage value.
    html { 
    /* Remove this */
    /* width: 1440px; */
    /* height: 900px; */
    . . . 
}

body {
    /* width: 100%; */
    /* height: 100%; */
    min-height: 100vh;
    display: flex;
    justify-content: center;
    align-items: center;
    . . .
}

.container {
    /* width: 50%; */
    /* height: 60%; */
    max-width: 900px;
    width: 100%;
    . . .
}
    • Instead of using pixels in font-size, use relative units like em or rem. The font-size in absolute units like pixels does not scale with the user's browser settings. This can cause accessibility issues for users who have set their browser to use a larger font size. You can read more about this here 📘.

    I hope you find it useful! 😄 Above all, the solution you submitted is great!

    Happy coding!

    Marked as helpful
    2
    Swing95•180
    @Swing95Posted almost 2 years ago

    @MelvinAguilar Thank you, but if I do not put height in html I can not figure out how to make body for example 900px height when content is 500px. It somehow does not work for me even if I put height 900px into body

    0

Please log in to post a comment

Log in with GitHub

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord