flexbox

Community feedback

• 

  Revin•280

  @revin212Posted almost 2 years ago

  Hello there, congrats for completing the challange!

  I noticed that you make the advice quotes manually by yourself, not fetching the data from the API. So I'll give you the script for fetching the data, and sowing it on the webpage :

  const adviceId = document.getElementById('advice-id');
  const adviceText = document.getElementById('advice');
  const diceBtn = document.getElementById('dice-btn');
  
  let id = 0;
  let advice = "";
  
  function displayAdvice(){
      let dataFetch = fetch('https://api.adviceslip.com/advice', {cache: "no-store"})
          .then((response) => response.json())
          .then((data) => {
              id = data.slip.id;
              advice = data.slip.advice;
              adviceId.innerText = `Advice #${id}`;
              adviceText.innerText = `"${advice}"`;
          });
  }
  displayAdvice();
  
  diceBtn.addEventListener('click', ()=> {
      displayAdvice();
  })
  

  You can set the id of each element that used here :

  id='advice-id' for the title (in your case, the p tag)

  id='advice' for the content (in your case, the div tag with 'content' class)

  id='diceBtn' for the dice button (in your case, the div tag with 'dice' class)

  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.

  • 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.

  Alternative text:

  • When you change the "<div class="dice">" to button, there is another problem: There isn't much information about what the button is for, and If the image does not contain alternative text, screen reader users will not be able to understand what the button does. You can use Generate new advice as the alt attribute value of the dice-icon. You can see an example here.

  • Not all images should have alt text. The divider image is a decorative image, it does not add any information to the page. You should use an empty alt attribute. You can read more about this here.

  • Since I can currently spam it with clicks, it would be better if you block the button and with the setTimeout() function enable the button again after 2 seconds.

  I hope you find it useful! 😄

  Happy coding! 🎄

  1