@GregLyons
Posted
Nice job! You're using fetch just fine here. I found asynchronous JS pretty tricky for awhile. You might find Sid's answer to this StackExchange question informative, as it's a pretty comprehensive answer. His breakdown of the Promise API (steps 1-6) is especially helpful.
You might also find async/await syntax more intuitive, which I use in my solution. It lets you, for example, assign the results of the asynchronous action to a variable in a "synchronous" looking manner. From my code:
**async** function getAdvice() {
const { id, advice, } = **await** fetch('https://api.adviceslip.com/advice')
.then(res => res.json())
.then(data => data.slip);
...
... (DOM stuff with my variables)
...
(Note that I'm using object unpacking to get the id and advice properties from data.slip.) Essentially, I'm able to store the returned value of the last then statement into variables. Using await means that my code won't go forward until the asynchronous action is complete. Without the await, what's returned would just be a Promise, not an object with id and advice attributes, and I couldn't use those variables.
To be clear, your solution is also completely correct/understandable. I just wanted to point out this new syntax since a lot of people find it easier to wrap their head around than the old methods of asynchronous programming. You might prefer Promise/then instead, and even if not, you'd still need to understand them a bit to use async/await.
One last thing I'll talk about is that your has a couple pesky scrollbars, even though they aren't necessary. They're not a big deal in this case, but they could be annoying in your future projects. Here's how you can get rid of them:
The vertical scrollbar
You set your <main> to have height: 100vh;, presumably to center your card vertically. Then, when you add your <footer>, this is additional content, so that the total space is greater than 100vh, hence the vertical scrolling. Centering things vertically is pretty tricky, and I can't think of a simple solution here. Where I would start would be putting height: 100vh on the <body> instead of the <main> (actually min-height: 100vh;; using height: 100vh would be bad practice since it could mess up when content actually does overflow the screen vertically) and then do some sort of positioning to get your footer at the bottom. (Maybe you could align-self it with Flex?) This article gives some helpful information on centering things vertically, though it doesn't cover your exact use case.
Anyway, that's where the vertical scroll bar is coming from.
The horizontal scrollbar
I believe this is because you're setting width: 100vw; on your main. I believe you're doing this to center the content horizontally, but I don't believe it should be necessary, as block elements (such as <div> and <p>) take up all the available horizontal space. I believe <main> is such an element, so it should already be taking up the width of the screen. I believe what's causing the horizontal scrollbar is that your vertical scrollbar from the previous point is adding on a bit of width to the page, so the width of your page is actually 100vw plus the width of the vertical scrollbar. That causes horizontal overflow.
I think removing width: 100vw; should fix this. In the future, when you do want scrolling but don't want it to mess with the width, you could use something like overflow-y: overlay;. This will make it cover any content below it though, so keep that in mind.
Hope this helps!
Marked as helpful
@geoffreyhach
Posted
@GregLyons thanks for the long and constructive review ! I’ll read this again in detail tomorrow to be sure to remember all of those things. Cheers !
@GregLyons
Posted
@geoffjecrois Happy to help!
