@andreasremdt
Posted
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 anh2
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 thebalance-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
@boonyarit-iamsaard
Posted
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 thebalance-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 π
@andreasremdt
Posted
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 div
s 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
@boonyarit-iamsaard
Posted
Hi @andreasremdt,
Now I understand clearly, thanksπ