Conversation
spitsfire
left a comment
spitsfire
left a comment
There was a problem hiding this comment.
Heck yeah, Airis and Doris! Good job using a build with your deployment!
Some things to consider:
- When assigning variables, don't forget your semicolon, even if the value is a function! Same when you call a function, too! It needs a semicolon at the end.
| <div id="landscape"></div> | ||
| </div> | ||
| </section> | ||
| <!-- <script src="./node_modules/axios/dist/axios.min.js"></script> --> |
There was a problem hiding this comment.
Looks like you used a build in order to import axios and other dependencies instead of relying on a JavaScript file! Great job!
Since we aren't using this anymore, let's get rid of it
| <!-- <script src="./node_modules/axios/dist/axios.min.js"></script> --> |
| @@ -0,0 +1,170 @@ | |||
| "use strict"; | |||
| import 'regenerator-runtime/runtime'; | |||
There was a problem hiding this comment.
Hm, where do we use this? Was this imported accidentally?
| gardenContent: null, | ||
| }; | ||
|
|
||
| const loadControls = () => { |
There was a problem hiding this comment.
Interesting approach! Very creative way to retrieve your state values all at once!
| } | ||
| } | ||
| } | ||
| loadControls() |
There was a problem hiding this comment.
This works! I suggest calling it inside our registerEventHandler function, so we know that everything has been loaded before we grab values from the DOM.
| loadControls() | |
| loadControls(); |
| const changeTempColor = (temp) => { | ||
| let color; | ||
| let backgroundColor; | ||
| if (temp >= 80) { | ||
| color = 'red'; | ||
| backgroundColor = "#a75051"; | ||
| } else if (temp >= 70 && temp <= 79) { | ||
| color = 'orange'; | ||
| backgroundColor = "#d3834d"; | ||
| } else if (temp >= 60 && temp <= 69) { | ||
| color = 'yellow'; | ||
| backgroundColor = "#c8a669"; | ||
| } else if (temp >= 50 && temp <= 59) { | ||
| color = 'green'; | ||
| backgroundColor = "#528478"; | ||
| } else if (temp < 50) { | ||
| color = 'teal'; | ||
| backgroundColor = "#4288c2"; | ||
| } | ||
| state.tempValue.setAttribute('class', color); | ||
| document.body.style.background = backgroundColor; | ||
| cityNameReset.style.backgroundColor = backgroundColor; | ||
| currentTempButton.style.backgroundColor = backgroundColor; | ||
| } | ||
|
|
||
| const changeLandscape = (temp) => { | ||
| let landscapeImage; | ||
| if (temp >= 80) { | ||
| landscapeImage = '🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂'; | ||
| } else if (temp >= 70 && temp <= 79) { | ||
| landscapeImage = '🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷'; | ||
| } else if (temp >= 60 && temp <=69) { | ||
| landscapeImage = '🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃'; | ||
| } else { | ||
| landscapeImage = '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲' | ||
| } | ||
| state.landscape.textContent = landscapeImage; | ||
| } |
There was a problem hiding this comment.
Another way to format these two methods (maybe combine them into one) would be to have an object that has temp values as keys, and then you could have values be a list. The list could hold background color and background image, like:
const tempDisplayObject = {
80: ['red', "🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂"],
70: ['orange', "🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷"]
} When you have an object, you don't need to do if/else if/else if/ else which can introduce bugs and also adds complexity to the method. If you have an object, than you can iterate over the object and check the keys with state.temp and then get the correct values to set the color and landscape emojis.
| const updateTempByCity = () => { | ||
| getLocationWeather(state.cityNameInput.value) | ||
| .then(temp => { | ||
| state.tempValue.textContent = temp; | ||
| changeTempColor(temp); | ||
| changeLandscape(temp); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Great job separating out these promises into individual functions and chaining them!
| const changeSky = () => { | ||
| let skyGraphic; | ||
| let gardenContentColor = 'garden__content '; | ||
|
|
||
| if (state.skySelect.value === 'sunny') { | ||
| skyGraphic = "☁️ ☁️ ☁️ ☀️ ☁️ ☁️"; | ||
| gardenContentColor += 'sunny' | ||
| } else if (state.skySelect.value === 'cloudy') { | ||
| skyGraphic = "☁️☁️ ☁️ ☁️☁️ ☁️ 🌤 ☁️ ☁️☁️"; | ||
| gardenContentColor += 'cloudy'; | ||
| } else if (state.skySelect.value === 'rainy') { | ||
| skyGraphic = "🌧🌈⛈🌧🌧💧⛈🌧🌦🌧💧🌧🌧"; | ||
| gardenContentColor += 'rainy'; | ||
| } else { | ||
| skyGraphic = "🌨❄️🌨🌨❄️❄️🌨❄️🌨❄️❄️🌨🌨"; | ||
| gardenContentColor += 'snowy'; | ||
| } | ||
| state.sky.textContent = skyGraphic | ||
| state.gardenContent.setAttribute('class', gardenContentColor) | ||
| } |
There was a problem hiding this comment.
Same as above! Let's turn this into an object, so we can cut down on all these if/else statements
| state[element] = document.getElementById(String(element)); | ||
| } | ||
| } | ||
| } |
| document.body.style.background = backgroundColor; | ||
| cityNameReset.style.backgroundColor = backgroundColor; | ||
| currentTempButton.style.backgroundColor = backgroundColor; | ||
| } |
There was a problem hiding this comment.
| } | |
| }; |
since this is an assignment on line 29, we give it a semicolon at the end of the expression
| landscapeImage = '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲' | ||
| } | ||
| state.landscape.textContent = landscapeImage; | ||
| } |
4 participants
No description provided.