Conversation
spitsfire
left a comment
spitsfire
left a comment
There was a problem hiding this comment.
Great job, Mica and Misha! So glad you tried out some more CSS grid styling! One thing I noticed is your class names, so I gave a little feedback there. Overall, you organized your event handlers well!
| <header class="titleBlock"> | ||
| <h1 id="siteTitle">The Weather Station</h1> | ||
| </header> | ||
| <span class="banner" id="skyBanner"></span> | ||
| <section class="cityNameBlock"> |
There was a problem hiding this comment.
Consider refactoring your class names so that they are lowercase and hyphenated between words
<h1 id="site-title">The Weather Station</h1>| const cityName = document.querySelector('#cityName'); | ||
| const inputContainer = document.querySelector('#inputCityName'); | ||
| state.city = inputContainer.value; | ||
| cityName.textContent = state.city; | ||
| }; | ||
|
|
||
| const resetCityName = (event) => { | ||
| const cityName = document.querySelector('#cityName'); |
There was a problem hiding this comment.
Looks like we are repeating code on lines 62 and 69! Maybe we can create global variables that will hold elements like this that will be used more than once
| axios | ||
| .get(`http://localhost:5000/location?q=${state.city}`) | ||
| .then((response) => { | ||
| state.latitude = response.data[0].lat; | ||
| state.longitude = response.data[0].lon; | ||
| console.log(response.data); | ||
| axios | ||
| .get( | ||
| `http://localhost:5000/weather?lat=${state.latitude}&lon=${state.longitude}` | ||
| ) | ||
| .then((response) => { | ||
| const temperatureKelvin = response.data.main.temp; | ||
| state.Temperature = ((temperatureKelvin - 273.15) * 9) / 5 + 32; | ||
| tempContainer.textContent = `${Math.round(state.Temperature)}`; | ||
| tempRange(); | ||
| console.log(response.data); | ||
| }) | ||
| .catch((error) => { | ||
| console.log('OpenWeather GET request error'); | ||
| }); | ||
| }) | ||
| .catch((error) => { | ||
| console.log('LocationIQ GET request error'); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
good indentation! makes the promises easier to read 👍
| if (state.Temperature >= 80) { | ||
| tempContainer.style.color = 'red'; | ||
| landscapeContainer.textContent = '🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂'; | ||
| } else if (state.Temperature <= 79 && state.Temperature >= 70) { | ||
| tempContainer.style.color = 'orange'; | ||
| landscapeContainer.textContent = '🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷'; | ||
| } else if (state.Temperature <= 69 && state.Temperature >= 60) { | ||
| tempContainer.style.color = 'yellow'; | ||
| landscapeContainer.textContent = '🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃'; | ||
| } else if (state.Temperature <= 59 && state.Temperature >= 50) { | ||
| tempContainer.style.color = 'green'; | ||
| landscapeContainer.textContent = '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲'; | ||
| } else if (state.Temperature <= 49) { | ||
| tempContainer.style.color = 'teal'; | ||
| landscapeContainer.textContent = '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲'; | ||
| } | ||
| }; |
There was a problem hiding this comment.
How might you refactor this so the logic is more data driven? Here you have several if/else if statements. What if you had an object with the keys as the temperature and the values as the landscape and temperature corresponding display? Then you could iterate over the object to set state!
It could look something like this:
const tempColors = [
[49, 'blue'],
[59, 'green'],
[69, 'gold'],
[79, 'orange'],
[null, 'red'],
];
const getColorForTemp = (temp) => {
for (const [boundaryTemp, color] of tempColors) {
if (boundaryTemp === null || temp <= boundaryTemp) {
return color;
}
}
};| @@ -0,0 +1,98 @@ | |||
| body { | |||
| display: grid; | |||
3 participants
No description provided.