Skip to content
OverviewSolutions
4
Comments
4
Dominik Gartz
@domieee

All comments

  • Dominik Gartzβ€’140
    @domieeePosted almost 2 years ago

    πŸ™‹πŸ»β€β™‚οΈ Hi Davi,

    I took a look at your code, and a few little things caught my eye.

    • Instead of working with values in rem, use max-width: 300px; & width 100%; on your container. then img get's width: 100%. I think this would be a bit more elegant.

    • In this case the text Scan the QR code to visit Frontend Mentor and take your coding skills to the next level would rather fit into a <p> element, as the content is not what a heading should contain. Headings should describe something like a page-, oder section content.

    • After your <main> element you should rather nest with section > article instead of <div>. Div's are no longer the right way for a good, reachable page

    To learn more about web accessibility, check out this link

    • The height: 30.938rem; on the container should not be necessary. It would be enough to let the content and its paddings and margins control the height.

    That's what I noticed so, still good job finishing the project!

    EDIT: I don't think you need Media Queries for this Project :)

    Have a nice day and keep on coding! (β€žα΅•α΄—α΅•β€ž)

    Marked as helpful
    1
  • Dominik Gartzβ€’140
    @domieeePosted almost 2 years ago

    πŸ™‹πŸ»β€β™‚οΈHi manta-lucian,

    I noticed only a few small things:

    • Instead of using div element, you should rather use a nesting with main > section. This is important for web accessibility. Take a look here

    • Instead of the fixed width of 300px, use max-width: 300px; + width: 100%. The img will keep the width: 100%;. This way everything remains responsive.

    P.S. Change the background-color property of your body 😜

    Edit: Also take a look at the h2 and change it to h1. h1 has to be the first heading in your code. Here's how you handle headings

    Haven't found anything else yet, maybe someone else will find something

    Otherwise great project and keep coding!◝(α΅”α΅•α΅”)β—œ

    Marked as helpful
    1
  • Dominik Gartzβ€’140
    @domieeePosted almost 2 years ago

    πŸ‘‹πŸΌ Hi Eduardo,

    I have looked at your code and I have noticed some things

    • For the web accessibility it is important that the h1 element is always at the beginning of the page. A web crawler always looks for the h1 element, and uses its content to describe what the page is about. Also you should nest your code into a <main>-element Take a look here

    • Don't use fixed heights or widths. You can give the container for the image a max-width: 320px; and a width: 100%. The img also gets width: 100%. So the container adapts to the browser size, and the image to the container. so everything stays responsive.

    • For the font-size property you should not specify px. Instead you can use rem. px does not adjust when the brower size is changed. Read more about rem here

    Otherwise it looks great and very similar to the design!

    Have a nice day and keep coding!

    Marked as helpful
    1
  • Dominik Gartzβ€’140
    @domieeePosted almost 2 years ago

    Hi Abdoulaye

    I took a look at your code and found a few things you could adjust.

    you have given the img element a height and width of 180px. This would be unresponsive if necessary and would not adjust to its parent element. You could fix this by giving the container a max-width of the desired width and the width: 100%;. The img should also have the property width 100%.

    for web accessibility you should add a <main role="main>" and nest your code in it. Also you should change your h3 element to an h1 element. this is also important for this case. You can take a look at google and see what is important for web accessibility.

    try to use rem instead of px for the font size. if necessary the font's wouldn't adjust to the brower size with px.

    Keep coding and have a great day

    Marked as helpful
    0