All comments

• 

  Manuela Torres Andion Vidal•70

  @mtavidalSubmitted over 1 year ago

  

  Advice generator using API, HTML, CSS and JS

  1

  30
  

  Robin Dervieux•230

  @robicode-05Posted over 1 year ago

  Hi !

  Well done for finishing this challenge ! I have some suggestions for you ^^

  HTML

  Keep an eye on the Accessibility report, on this page. But mainly, you should use semantic html tag, like the main to encapsulate your content.

  the heading element should decrease, in your case, the h3 and h2 are interverted.

  JS

  In the function newAdvice(), the url is being redeclare on every call.
  Since this value won't ever change, it can be declared outside the function scope :

  (bonus tip: in Javascript, if the variable is never reassign, prefer const over let, it will let the web browser have a better optimization)

  const url = "https://api.adviceslip.com/advice"
  async function newAdvice () {
    try {
      const response = await fetch(url);
      const data = await response.json();
      showAdvice(data);
    } catch (error) {
      console.error(error);
    }
  }
  

  ---

  the function showAdvice() is setting up the whole text "Advice #---", but Advice is a static text an never change overtime. So it's a good practice to include this text directly in the HTML, and use Javascript to update only what's necessary

  ex:

    <h3>
      <span>ADVICE #</span>
      <span id="adviceNumber"> </span>
    </h3>
  

  function showAdvice(data) {
      const adviceNumberShow = document.getElementById("adviceNumber");
      adviceNumberShow.textContent = data.slip.id;
      const adviceTextShow = document.getElementById("adviceText");
      adviceTextShow.textContent = `"${data.slip.advice}"`;
  }
  

  ---

  In the js file there is this code :

  x = window.screen.width;
  
  if (x <= 428) {
      document.getElementById('line').innerHTML = ....;
      document.getElementById('line').style.width = "295";
  }
  

  and this won't work. Since the x value is set on startup, it will never change if the page is resize. It will work on first load, but this is really not a good practice !

  To handle multiple screen size, try to use CSS most of the time.

  If you wan't to update content dynamically, you can use :

  • the picture tag with embedded media queries : <picture>: The Picture element / MDN

  exemple :

  <picture>
    <source srcset="https://via.placeholder.com/1400" media="(min-width: 1400px)"/>
    <source srcset="https://via.placeholder.com/1200" media="(min-width: 1200px)"/>
    <source srcset="https://via.placeholder.com/800" media="(min-width: 800px)"/>
    <source srcset="https://via.placeholder.com/600" media="(min-width: 600px)"/>
    <img src="https://via.placeholder.com/400" alt="example"/>
  </picture>
  

  • Or the css property background-image : background-image / MDN

  If you REALLY REALLY need to use Javascript to react on resize,
  I strongly recommande using a ResizeObserver. It will keep track of resize events, but with bettre performance than just adding a listener to rezize event.

  Otherwise congratulations on finishing this project, and have fun on the next one ;)

  0
• 

  P

  Donta Smith•40

  @donta514Submitted over 1 year ago

  Any tips or feedback regarding best practices is greatly appreciated :)

  

  QR code component

  2

  20
  

  Robin Dervieux•230

  @robicode-05Posted over 1 year ago

  Hello !

  In addition to what @Hassiai said, you do not need to redefine font-family for all the text.

  font-family in css in one of the few property which is apply on every descendent automatically. So the easiest way is to define it inside html selector.

  html {
     font-family: "Outfit", sans-serif;
  }
  

  Have fun for your next challenge ;)

  Marked as helpful

  0