Advice generator app using React and Advice Slip API

Community feedback

• 

  P

  Andreas Remdt•950

  @andreasremdtPosted over 1 year ago

  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

  @andyjv1Posted over 1 year ago

  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

  @andreasremdtPosted over 1 year ago

  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
• 

  T

  Grace•31,270

  @grace-snowPosted over 1 year ago

  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

  @andyjv1Posted over 1 year ago

  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