Sea Turtles - Tiffini H. #68
Conversation
…o async programming)
…ll needs troubleshooting)
| if (state.temperature >= 80) { | ||
| landscapeImage.src = 'assets/marissa-rodriguez-summer.jpg'; | ||
| landscapeImage.alt = 'Crystal clear turquoise water in a pool or sea'; | ||
| landscapeCaption.textContent = 'Photo by Marissa Rodriguez'; |
There was a problem hiding this comment.
I really like how you handled attribution for the photos!
| } else if (state.temperature >= 20 && state.temperature <= 39) { | ||
| state.currentColor = '#1a557d'; | ||
| } else { | ||
| state.currentColor = '#67686b'; |
There was a problem hiding this comment.
In production code we'd likely want to include comments about what each of the hex colors are, since many folks cannot sight-read hex values.
There was a problem hiding this comment.
Ahh, that makes sense. Thank you!
| <body> | ||
| <div class="container"> |
There was a problem hiding this comment.
I recommend replacing this div with a main element, since main is intended to represent the significant content of a page. Depending on if it significantly changes the layout, another option might be to move class="container" to the body tag and remove this div.
There was a problem hiding this comment.
Thank you, Kelsey! I thought that this div would maybe be okay since I was grouping just for styling purposes, but I can see how a main tag would have made more sense.
| </section> | ||
|
|
||
| <section id="sky-and-landscape-container"> | ||
| <figure id="sky-with-caption"> |
There was a problem hiding this comment.
Nice use of the figure element!
| @@ -0,0 +1,238 @@ | |||
| 'use strict'; | |||
|
|
|||
| const state = { | |||
There was a problem hiding this comment.
Nice use of the state object!
| const skyCaption = document.getElementById('sky-caption'); | ||
| const selectedSky = document.getElementById('sky-dropdown').value; | ||
|
|
||
| state.currentSky = selectedSky; |
There was a problem hiding this comment.
We update state.currentSky here, but it doesn't look like that value is read anywhere in the file. We can probably remove currentSky from state so we don't hold onto data we won't use.
There was a problem hiding this comment.
Ooh, that's a great catch!
|
|
||
| const currentTemp = document.getElementById('temperature'); | ||
|
|
||
| // update display temp, sky, and landscape after retrieving weather data |
There was a problem hiding this comment.
Nice use of comments throughout!
| alt="Crystal clear turquoise water in a pool or sea"> | ||
| <figcaption id="landscape-caption">Photo by Marissa Rodriguez</figcaption> | ||
| </figure> | ||
| </section> |
There was a problem hiding this comment.
Overall, nice division of content into sections.
There was a problem hiding this comment.
Thank you! I worked really hard to make my html as thoughtful as possible based on your feedback about how HTML can impact accessibility, so I'm glad it's an improvement!
| return [lat, lon]; | ||
| }) | ||
| // get current temp of current city | ||
| .then((response) => { |
There was a problem hiding this comment.
Nice use of .then chaining to fire the calls off in sequence! This function is a bit long, if we wanted to divide it up for clarity, we could move the /weather endpoint call to its own function and call the new function from a .then block here.
There was a problem hiding this comment.
Ooh, I'd love to talk about this when we check in next!
| skyDropdown.addEventListener('change', manuallyChangeSky); | ||
| }; | ||
|
|
||
| document.addEventListener('DOMContentLoaded', registerEventHandlers); |
There was a problem hiding this comment.
It's easy to overlook this line nestled between functions. I recommend moving it to the very bottom to keep functions together and separate from code that we immediately call and execute.
No description provided.