Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

Submitted

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

john 90

@johnhaab

Desktop design screenshot for the Advice generator app coding challenge

This is a solution for...

  • HTML
  • CSS
  • JS
  • API
2junior
View challenge

Design comparison


SolutionDesign

Solution retrospective


Feedback appreciated.

Learning JS so its not the best.

Community feedback

@caarlosdamian

Posted

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

@MelvinAguilar

Posted

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

@Bhikule19

Posted

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

@johnhaab

Posted

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

0

Please log in to post a comment

Log in with GitHub
Discord logo

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord