Responsive advice generator app

Community feedback

• 

  Mohamed ELIDRISSI•455

  @elidrissidevPosted over 2 years ago

  Hey Naveen 👋,

  Great work on your solution, keep on going!

  I have some feedback for you, first is an accessibility one:

  • The dice button doesn't have any descriptive text inside. For accessibility reasons, buttons and links must always have some text inside that describes their action, in the case where it's not a part of the design, consider making the text visually hidden.
  • The dice image is mainly for decoration, and when you add a proper title ☝️, there will be no need for accessibility software to announce the alt text, in this case, you can make it empty so that it's not announced.

  This one is coding-style related:

  • Try to keep things consistent in your code so other people find it easier to read. For example, you have some random spaces in your JSX between the attribute and value. In general, try to follow the common conventions, in this case, no spaces between attribute and its value in JSX.

  This one is related to React:

  • Know when and what to separate something to its own component. For example, you have created an Advice component, but it only contains an h1 and the rest including the button is in the App component. What I would do in this case is extract the whole card to the Advice component.
  • I would also recommend separating data fetching logic to a custom hook. For example, you can create a hook called useAdvice and inside of it you would put the state and the fetching function, you would then return those to be consumed by the component. e.g:

  function useAdvice() {
    const [advice, setAdvice] = useState(null);
    const [isLoading, setIsLoading] = useState(false);
    const [error, setError] = useState(null);
  
    const fetchAdvice = () => {
      setIsLoading(true);
      axios.get("https://api.adviceslip.com/advice")
        .then((res) => 
          {
            setAdvice(res.data.slip); 
            setIsLoading(false);
            setError(null);
          })
        .catch((error) => 
          {
            console.log(error);
            setError(error);
            setIsLoading(false);
          });
    }
  
    return { advice, isLoading, error, fetchAdvice }
  }
  

  • I would remove the hard-coded advice that's shown initially on page load, instead you can replace it by a useEffect with an empty dependency array that will fire when the component mounts and fetch the initial advice from the API.

  Phew, that was a lot 😅. Hope it helps, Good luck!

  Marked as helpful

  1

  ---

  

  Naveen Ongole•80

  @Naveen39OPosted over 2 years ago

  @elidrissidev Thanks for the detailed review and ideas for improvement.

  0