Advice Generator using API with HTML, CSS and JS

Community feedback

• 

  T

  Chamu•13,130

  @ChamuMutezvaPosted over 2 years ago

  Hi MightyKale’s. So far so good with your first challenge.

  • the display looks generally good but as the device gets smaller the image divider is not scaling well - it is overlapping the container.
  • the recommended way for dealing with responsive images is to use methods such as the srcset and picture element. Controlling the images between sizes by using display: none works, but it is not the best practice. Some of the reasons why using display: none to interchange the images is because it wastes bandwidth as the html is parsed first - hence all the images will be downloaded, making it an expensive method. That can cause you project to load slow when the site has a lot of images. I normally use the picture element, here is a site that can get you started Responsive Images 101
  • the first heading element of your site should be an h1, heading elements should follow a sequential order without skipping heading elements.
  • use semantic elements where possible, one case is where you used the div for interactive purposes. Click events should be associated with an element such as a button , a div is not recommended to use - it has no semantic meaning. Assistive technology users will not be able to interact with your site.
  • the advice message is not changing if i click the dice. Try to clear the cache by editing the fetch statement as follows `const response = await fetch(apiUrl, {cache: "no-cache"});

  Marked as helpful

  1

  ---

  

  MightyKale•40

  @MightyKalePosted over 2 years ago

  @ChamuMutezva Thank you so much for your feedback. The Responsive Images 101 link you provided is very helpful, and I will look into that and also the semantic elements. I was wondering in my case, should I change the attribution div to a <footer>? I can't seem to find other places where I can use semantic elements for this challenge, please let me know if you have any suggestions, thank you!

  0

  

  T

  Chamu•13,130

  @ChamuMutezvaPosted over 2 years ago

  @MightyKale, yes change the div with the class of attribution to a footer element. You also need to have a main element - you can use the container div as the main element

  Marked as helpful

  1
• 

  Eslam Ismail•390

  @eslam-94Posted over 2 years ago

  great work

  0
• 

  sansarj17•180

  @sansarj17Posted over 2 years ago

  Hello Kale, Congrats on completing the challenge.

  My suggestion would be to light up the dice when the person hovers on it so the user knows that he can interact with it. As for the divider, I think using anyone would have been fine and you can scale up/down the divider image according to the viewport. However, I do like how you tackled the issue.

  0

  ---

  

  MightyKale•40

  @MightyKalePosted over 2 years ago

  @sansarj17 Hello Sansarj, thanks for your reply! I might have misunderstood the challenge so the glowing effect is only present when the button is active.

  0