@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 inApp.jsx
and the other inDicecontainer.jsx
. This is duplicated code that you should try and avoid. Ideally, you keep the fetch function insideApp.jsx
and trigger it with a prop likeonClick
, which you pass intoDicecontainer
. You can then remove thefetchAdvice
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 asid
. Changing a variable type rarely a good thing, so consider changing the initial state to{}
ornull
.
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
@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??
@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.