Skip to content
OverviewSolutions
13
Comments
2

Robin Dervieux

@robicode-05230 points

I’m a mysterious individual who has yet to fill out my bio. One thing’s for certain: I love writing front-end code!

Latest solutions

View more solutions

Latest comments

  • Robin Dervieux•230
    @robicode-05

    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 :

    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>

    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 ;)

  • Robin Dervieux•230
    @robicode-05

    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