Expenses chart component using HTML, CSS, and Vanilla JavaScript

Community feedback

• 

  P

  Andreas Remdt•950

  @andreasremdtPosted over 2 years ago

  Hey @boonyarit-iamsaard,

  Congrats on solving the challenge, it's looking really good and close to the design. I like that you didn't use any library or framework for the chart, nice practice exercise :)

  A couple of things:

  • You don't have any heading in your markup, which could lead to accessibility and SEO issues. I would use an h1 for My balance and an h2 for Spending - Last 7 days (though others might reverse the order or use different levels, there's room for discussions).
  • You could use a header element for the balance-container, would make it even more semantically correct.
  • In terms of your JavaScript and data fetching I can't really find anything that needs dior improving, the error handling is solid and everything is nicely structured into reusable functions. One addition I can recommend is checking the HTTP status via if (response.ok) {...}, because a 404 or many other HTTP "errors" don't throw errors, but your script would still fail. It's always good to think about these edge-cases. So, you could do something like this:

  try {
      const response = await fetch(url);
  
      if (!response.ok) {
          throw new Error(`${response.status}: ${response.statusText}`);
      }
  
      return await response.json();
    } catch (error) {
      throw new Error('Expenses data is not available.');
    }
  

  Otherwise, nicely done :)

  Marked as helpful

  1

  ---

  

  Boonyarit Iamsa-ard•80

  @boonyarit-iamsaardPosted over 2 years ago

  Hi @andreasremdt,

  First of all, I thank you for your feedback, it means a lot to me as I'm self-taught.

  Here are replies to your feedback:

  • I'm not so sure where to put the level heading in my markup, so I use the .sr-only class with it instead. Is that a good practice?
  • I couldn't agree more to use the header element for the balance-container.
  • I was thinking to check response.ok when I'm doing the JavaScript part, but I skip it because I thought the Fetch API will throw the HTTP error to the catch block.

  Thanks again for the feedback 👍

  1

  

  P

  Andreas Remdt•950

  @andreasremdtPosted over 2 years ago

  Hey @boonyarit-iamsaard,

  You are welcome :) In regards to your heading: using a .sr-only element is one way of solving it, but the even easier solution would be to replace the divs around My balance and Spending - Last 7 days with heading elements. I would use an h1 for My balance, as to me this seems to be the most important title and it's also the first one on the page (both visually and semantically). And for Spending - Last 7 days, I'd recommend using an h2 instead, since it comes below but marks the title of the chart section, so it's also kinda important. Hope this helps.

  Marked as helpful

  1

  

  Boonyarit Iamsa-ard•80

  @boonyarit-iamsaardPosted over 2 years ago

  Hi @andreasremdt,

  Now I understand clearly, thanks👍

  0