Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

Submitted

Responsive Landing Page - SCSS

Fat 850

@Fahatmah

Desktop design screenshot for the Sunnyside agency landing page coding challenge

This is a solution for...

  • HTML
  • CSS
  • JS
2junior
View challenge

Design comparison


SolutionDesign

Solution retrospective


How can I use the picture tag for a different size of images in this landing page? I tried it but it was just displaying two image even though I put a media attribute with different values just like this:

<picture>
  <source media="(min-width: 375px)" srcset="...mobile-image.jpg">
  <source media="(min-width: 1440px)" srcset="...desktop-image.jpg">
  <img src="...mobile-image.jpg" alt="">
  <img src="...desktop-image.jpg" alt="">
</picture>

Community feedback

PhoenixDev22 16,990

@PhoenixDev22

Posted

Hello

Congratulation on finishing this challenge. Excellent work! I have few suggestions regarding your solution, if you don't mind:

HTML

  • It’s not recommended to add event listener on non-interactive elements class="menu-open". You can use a <button> with type=”button” around the img.
  • As it has no discernible name, put an aria-label on your trigger to describe its purpose. For example, you can have: aria-label='Mobile Navigation Trigger' or 'Open Menu.’
  • Add and toggle an aria-expanded attribute to your toggle element letting the user know if the content the button controls is expanded or not. At first, it has the “false” as a value then you use JavaScript to change the value.
  • You should use aria-controls attribute on the toggle element, it should reference the id value of the <ul> element.
  • Don't capitalize in html, let css text transform take care of that. Remember screen readers won't be able to Read capitalized text as they will often read them letter by letter thinking they are acronyms.
  • Profile images like that avatar are valuable content images, not decorative, so should have alt text eg " Jennie F"
  • You should add aria-hidden=”true” to the images that make them be ignored by screen readers to avoid redundancy and repetition.

Aside these, great job on this one. hopefully this feedback helps.

Marked as helpful

0

Fat 850

@Fahatmah

Posted

@PhoenixDev22 Thank you so much! I've been wanting to improve things in my markup but I don't know where to start or what needs to be improve until you suggested this things. I don't really mind it instead I am happy that someone gave me suggestions.

  • For the toggle menu, I only used image because I thought button cannot be nested with an anchor tag that has an image due to the last time in my html issues in my report from a fem challenge
  • I don't know what is aria-expanded is and how it should be coded in Javascript. Can you give me some example?
  • And aria-controls with the referencing of id of ul 😅

I have a lot to learn. And I am willing to learn it all. 😊

0
David 8,000

@DavidMorgade

Posted

Hello Fahatmah, congrats on finishing the challenge!

The problem is that you are setting two img tags at the bottom of your picture tag, try using one instead like this :

        <picture>
          <source   media="(min-width: 375px)" srcset="./assets/mobile/image-gallery-milkbottles.jpg">
          <source  media="(min-width: 1440px)" srcset="./assets/desktop/image-gallery-milkbottles.jpg">
          <img width="100%" src="./assets/desktop/image-gallery-milkbottles.jpg" alt="">
        </picture>

Anyway this video help me a lot to understand properly the picture tag, check it out!

Also sent you a pull request so you can see the changes (sorry for the inline styles, I don't have SASS compiler or any tool installed to work with SCSS right now!)

Hope this helps!

Marked as helpful

0

Fat 850

@Fahatmah

Posted

@DavidMorgade Hey David, thank you for the answer. I tried it and it is switching but the thing is, when the browser's size is in 375px, it displays the desktop image but when the size increases it switched to the mobile image which is kind of inverted. Anyway thank you for the video I get to know more about the picture tag and thank you!

1
David 8,000

@DavidMorgade

Posted

@Fahatmah Did you remove the hidden classes that you defined in your _pictures.scss ?

0
Fat 850

@Fahatmah

Posted

@DavidMorgade Actually I tested it in another document without any styles. But for sure I will learn from it. 😊

0
Fat 850

@Fahatmah

Posted

@DavidMorgade Hey David! I finally figured it out! Thanks for the pr, I still don't know how to utilize the other functions of github hehe.

Based from your pr it says that the desktop image comes after the mobile image and still don't work what it should be instead it was kind of inverted but I still appreciate your feedback! Thanks to you and Kevin Powell.

I tried what Kevin Powell's way of writing the markup of the picture tag and the source, and it works!

This was the code:

<picture>
    <source media="(min-width: 500px)" srcset="./assets/desktop/image-gallery-milkbottles.jpg">
    <source srcset="./assets/mobile/image-gallery-milkbottles.jpg">
    <img width="200px" src="./assets/desktop/image-gallery-milkbottles.jpg" alt="">
  </picture>

I changed the width to make it small and to see the switching of images and I think the most important part is placing the desktop image as the first in line and the mobile image comes after. Also the media attribute in mobile image was also removed as it tells the browsers as the default image.

This was fun to learn! I could use this in my future project. Once again, Thank You!

0

Please log in to post a comment

Log in with GitHub
Discord logo

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