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 app using React and Advice Slip API

#accessibility
andyjv1• 320

@andyjv1

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


I had a few issues with the API and the button while building this project. It seems to be fixed now. Any Feedback is welcomed.

Community feedback

P
Andreas Remdt• 950

@andreasremdt

Posted

Hey @andyjv1,

Nice job finishing this challenge! Since @grace-snow already mentioned some things, I'll purely focus on the React/JavaScript part:

  • You have 2 fetchAdvice functions, one in App.jsx and the other in Dicecontainer.jsx. This is duplicated code that you should try and avoid. Ideally, you keep the fetch function inside App.jsx and trigger it with a prop like onClick, which you pass into Dicecontainer. You can then remove the fetchAdvice inside this component and connect the prop with the button's click event.
  • The logic inside fetchAdvice can be improved. You don't need a timeout to disable/enable the button, this should be depending on the state of the promise. Consider something like this:
  const fetchAdviceClicked = async () => {
    setDisabled(true);
    const API_LINK = "https://api.adviceslip.com/advice";
    const response = await fetch(API_LINK);
    const advice = await response.json();
    props.setText(advice.slip)
    setDisabled(false);
  };

Right before the request is made, you disable the button. Once the request went through (and assuming it was successful), you enable it again.

  • You don't have any error handling in your code. As an example, what happens if the request can't be made because the user is offline? In the DevTools, you'd see an error, but the common user doesn't have DevTools open :D Try thinking about ways to show an error, even if it's not part of the design. As an example, you could use try/catch:
  const fetchAdviceClicked = async () => {
    try {
        setDisabled(true);
        const API_LINK = "https://api.adviceslip.com/advice";
        const response = await fetch(API_LINK);
        const advice = await response.json();
        props.setText(advice.slip)
        setDisabled(false);
    } catch (ex) {
        setError("Something went wrong...");
    }
  };
  • Even though you disabled the button while disabled is truthy, it still has a hover state, which is confusing. Consider adding hover and focus states only when the button is enabled, otherwise, just gray it out. One way to solve this would be by using CSS pseudo classes: button:not(:disabled):hover { ... your code here }
  • Your initial state in App.jsx is an empty array, but the advice slip is an object with properties, such as id. Changing a variable type rarely a good thing, so consider changing the initial state to {} or null.

Let me know if you have any questions, I hope my comments are helpful. Keep up the good work and happy coding!

Marked as helpful

0

andyjv1• 320

@andyjv1

Posted

Hi @andreasremdt , Thanks for the helpful comments. One questions the reason why i put a timeout is because the advice is cached for 2 seconds. When i remove the timeout, i can click multiple times without the advice changing. How do i navigate this??

0
P
Andreas Remdt• 950

@andreasremdt

Posted

Hey @andyjv1,

I had a similiar issue during the development of this challenge. Weirdly enough, I only encountered it in Firefox, not Chrome. What worked for me was to set cache: reload in the Fetch API, see my solution code here.

Let me know if this worked for you.

0
P
Grace• 27,870

@grace-snow

Posted

Hi

You need to change the HTML in this. It's not accessible / appropriate at the moment

  1. Advice # is the heading
  2. The advice text itself is a paragraph or could use a blockquote
  3. Use the picture element for responsive images not 2 img elements
  4. The line is decorative so alt must be blank
  5. The button must be properly labelled with what clicking it will do. I would do this with text inside the button and leave the button img alt blank
  6. Because some assistive tech users will not know that advice has changed after clicking the button I recommend wrapping everything in an aria-live element (eg add the attribute to smallcontainer)

Some of the styling looks very strange on mobile with the advice box positioned half off the screen. I wouldn't expect this design to need a media query at all tbh. I will add some images to discord of what I see. It's probably the width 80% on larger screens and the margin top on smaller screens both causing issues

However you definitely should be working mobile first even if this does need a media query.

And letter spacing must never ever be in px! i recommend em

Marked as helpful

0

andyjv1• 320

@andyjv1

Posted

hi @grace-snow , im working on your comments. One thing, i dont really understand point 5. How can i put text in the button without showing the text on screen? Thanks

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