Conversation
Co-authored-by: beenishali693 <[email protected]>
Co-authored-by: nelasunitha <[email protected]>
Co-authored-by: nelasunitha <[email protected]>
Co-authored-by: beenishali693 <[email protected]>
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good! Please review my comments, and let me know if you have any questions. Nice job!
| state.cityNameReset = document.getElementById('cityNameReset'); | ||
| }; | ||
|
|
||
| let temperature; |
There was a problem hiding this comment.
Why have this additional temperature value when there's a tempValue field in your state?
| state.cityNameReset.addEventListener('click', resetCityName); | ||
| }; | ||
|
|
||
| document.addEventListener('DOMContentLoaded', registerEventHandlers); |
There was a problem hiding this comment.
Even though we include this script at the end of the HTML file, it's still good practice to wait to kick off the page logic until we get the DOMContentLoaded event, as you did here.
Note that the function doesn't need to be called registerEventHandlers. So we could have had a function, maybe setupPage that calls loadControls and registerEventHandlers (which could now focus exclusively on registering event handlers rather than first loading the controls).
|
|
||
| const increaseTemp = () => { | ||
| ++state.tempValue; | ||
| document.getElementById('tempValue').textContent = state.tempValue; |
There was a problem hiding this comment.
We could have stored a reference to tempValue in the state, just like for the controls we looked up for registering events. It's pretty common to store references both to controls we use for event handling, as well as those where we need to change the output values.
| ++state.tempValue; | ||
| document.getElementById('tempValue').textContent = state.tempValue; | ||
| temperature = state.tempValue; | ||
| changeColorByTemperature(temperature); |
There was a problem hiding this comment.
We could pass state.tempValue as the parameter directly.
changeColorByTemperature(state.tempValue);| document.getElementById('tempValue').textContent = state.tempValue; | ||
| temperature = state.tempValue; | ||
| changeColorByTemperature(temperature); |
There was a problem hiding this comment.
Consider moving this logic related to updating the UI based on the current state.tempValue to a helper function (maybe publishTemperature). This could be called from both the increase and decrease handlers (as well as anywhere else we want to be certain the UI is in agreement with out current state.tempValue).
| const changeSky = () => { | ||
| let skyOption = state.skySelect.value; | ||
| if (skyOption === 'sunny') { | ||
| document.body.style.backgroundImage = 'url(../assets/sunny_sky.jpg)'; |
There was a problem hiding this comment.
Prefer to set a class, and then use CSS rules to update individual proerties, such as the backgroung-image and background-size.
|
|
||
| const changeSky = () => { | ||
| let skyOption = state.skySelect.value; | ||
| if (skyOption === 'sunny') { |
There was a problem hiding this comment.
How could we simplify this logic if we stored our sky data in a structure something like
const skyOptions = {
sunny: 'sunny-sky',
cloudy: 'cloudy-sky',
rainy: 'rainy-sky',
snowy: 'rainy-sky',
};which has the displayed sky option as the keys, and a class to apply as the values.
| cityNameReset: null, | ||
| }; | ||
|
|
||
| const loadControls = () => { |
There was a problem hiding this comment.
In addition to looking up the elements on whcih we want to register event handlers, we could also look up the elements we'll use to display values.
| let temperature; | ||
|
|
||
| const changeCityName = () => { | ||
| headerCityName.innerText = state.cityNameInput.value; |
There was a problem hiding this comment.
Prefer textContent rather than innerText.
| document.getElementById('tempValue').textContent = state.tempValue; | ||
| tempValue.style.color = 'teal'; | ||
| landscape.textContent = '❄️🥶☃️❄️🥶☃️❄️🥶☃️❄️🥶☃️❄️🥶☃️'; |
There was a problem hiding this comment.
As mentioned below, we could move the code responsible for updating the UI based on the temperature to a helper that could be called from all the places where we need to refresh the UI based on changes to the temperature.
3 participants
No description provided.