Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great looking site Muniba & Liz! I've left a few comments, once you take a look please let me know if there's anything I can clarify =]
| </div> | ||
| <button id="cityNameReset" class="city-name__reset-btn">Reset</button> | ||
| </section> | ||
| <section class="footer__content"> |
There was a problem hiding this comment.
To follow best practices for semantic HTML, if this represents a footer, we should use the <footer> tag and style as needed.
| <h2 id="headerCityName" class="header__city-name">Las Vegas</h2> | ||
| </div> | ||
| <span>Weather Last Updated: <span id="lastUpdatedValue">N/A</span></span> | ||
| <div class="temperature__content"> |
There was a problem hiding this comment.
I would strongly suggest swapping out some of the divs across the page for more semantic elements like nested sections or articles, and consider using a main tag to make it clear what is the central content of the page for folks using assistive technologies.
| city: 'las vegas', //default city | ||
| weather: {}, | ||
| } | ||
| const temperatureMappings = [ |
There was a problem hiding this comment.
I love this for making it easy to see what the temperature settings are all in one place.
| const elementsWithId = document.querySelectorAll('[id') | ||
| elementsWithId.forEach(element => state[element.id] = element); |
There was a problem hiding this comment.
If you know you want every element with an ID loaded up at the start, this is such a handy way to do it!
| state.weather.sunset = convertTimestampToTime(sunset); | ||
| state.lastUpdated = convertTimestampToTime(weatherResponse.data['dt']); | ||
|
|
||
| // update UI |
There was a problem hiding this comment.
Great use of short comments to help organize the code and give readers context
| } | ||
|
|
||
| const handleCityNameUpdate = () => { | ||
| state.city = state.cityNameInput.value || 'Las Vegas'; |
There was a problem hiding this comment.
I love this to handle when the user might clear the input field.
| const handleIncrease = () => handleTempChange(1); | ||
| const handleDecrease = () => handleTempChange(-1); |
There was a problem hiding this comment.
These are declared pretty far from where they're used, I'd consider moving them closer to the function that registers these as callbacks. Since they are only used in one location, another option could be writing them as anonymous functions where they are registered:
state.increaseTempControl.addEventListener('click', () => {
handleTempChange(1)
});| document.documentElement.style. | ||
| setProperty('--bgImage', `var(--${state.timePeriod}BgImage)`); | ||
| document.documentElement.style. | ||
| setProperty('--bgColor', `var(--${state.timePeriod}BgColor)`); | ||
| state.sky.src = `./src/icons/01${state.timePeriod}.svg`; |
There was a problem hiding this comment.
So good how the background updates with the real environment ^_^
| setTimePeriod(); | ||
| }; | ||
|
|
||
| document.addEventListener('DOMContentLoaded', onLoaded); |
There was a problem hiding this comment.
Really nice organization of code across the file!
3 participants
We completed the base project in the digital-template branch. We then added additional styling and functionality in the styling branch, which is the branch this pull request was made from.