Advice generator -- HTML & CSS & JS & API.

Community feedback

• 

  Carlos Damian Perez•1,890

  @caarlosdamianPosted almost 2 years ago

  Hello John Good job on completing the challenge ! I have some feedback for you if you want to improve your code.

  HTML:

  Use the <main> tag to wrap all the main content of the page instead of the <div class="container"> tag. With this semantic element you can improve the accessibility of your page.

  You can use an </hr> instead of and img on line 32-34 index.html

  Javascript:

  Function randomNum takes to two parameters but you dont pass those instead you defined in the body of the function remove the parameters and change the function like this

  function randomNum() {
      let min = 1;
      let max = 224;
      return Math.floor(Math.random() * (max - min + 1) + min);
  }
  

  Instead of repeat yout code you can just create a function that fetch the data and jus call it with the randomNumber like fetchData(randomNum()) as well use template strings on your url string you can read more about it here Template_literals

  const fetchData = (ranNum) => {
  fetch(
      `https://api.adviceslip.com/advice/${ranNum}`
  
  ).then(response => {
      return response.json();
  }).then(adviceData => {
      const adviceObj = adviceData.slip;
      id.innerText = adviceObj.id;
      id.style.letterSpacing = '3rem;';
      advice.innerText = '"' + adviceObj.advice + '"';
      advice.style.fontWeight = '900';
      advice.style.fontSize = '1.8rem';
  }).catch(error => {
      console.log(error);
  });
  }
  
  
  I hope you find it useful! 😄
  
  Happy coding!
  

  Marked as helpful

  1
• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted almost 2 years ago

  Hi there 👋. Good job on completing the challenge ! I have some feedback for you if you want to improve your code.

  HTML:

  • Use the <main> tag to wrap all the main content of the page instead of the <div> tag. With this semantic element you can improve the accessibility of your page.

  • You must use a level-one heading (h1) even though this is not a full-page challenge

  • Adding functionality to non-interactive elements like div or img is not recommended because they are not designed to be interactive. You should use interactive elements like button to make your elements interactive.

  • When you change "<div class="card-footer" id="btn">" to a button there is another problem: There isn't much information about what the button is for, and dice icon as the alt attribute value is not very descriptive, screen reader users will hear "Graph, dice" and they won't understand what the button does. You can use Generate new advice as the alt attribute value. You can see an example here.

  • The Advice Slip JSON API documentation says that the repeat requests within 2 seconds will return the same response. You have two options to solve this problem:

    • -1. Use the cache: no-cache option in the fetch function to receive a new response every time: fetch(your_url, { cache: "no-cache" }) and use the setTimeout function to prevent the user from spamming the button.
    • -2. Block the button and with the setTimeout function enable the button again after 2 seconds.

  • The width: 100% property in the body tag is not necessary. The body tag is a block element and it will take the full width of the page by default.

  • Use min-height: 100vh instead of height: 100vh. The height property will not work if the content of the page grows beyond the height of the viewport.

  • Centering an element with position: absolute would make your element behave strangely on some screen sizes, "there's a chance the content will grow to overflow the parent". You can use Flexbox or Grid to center your element. You can read more about centering in CSS here.

  I hope you find it useful! 😄 Above all, the solution you submitted is great!

  Happy coding!

  Marked as helpful

  1
• 

  Abhishek Gulab Bhikule•200

  @Bhikule19Posted almost 2 years ago

  Amazing work there, just to let you know the picture tag syntax can be more meaningful as it is used to manipulate img for different screen sizes.

  You can take a look at this how you can write picture tag the better way:-

  <picture>
          <source
            media="(max-width: 450px)"
            srcset="./images/pattern-divider-mobile.svg"
          />
          <img
            src="./images/pattern-divider-desktop.svg"
            alt="divider pattern svg"
            class="divider"
          />
        </picture>
  

  I hope you found it useful

  Marked as helpful

  0
• 

  john•90

  @johnhaabPosted almost 2 years ago

  LOOKS FINE ON WEBSITE, LOCALHOST, MY PHONE BUT NOT THIS LOL WHAT

  0