Panther - Ryan Thomas #95
Conversation
| <h1>Today's Weather</h1> | ||
| <span>Today in the city of | ||
| <span id="headerCityName" class="header__city-name"> | ||
| "insert city" | ||
| </span> | ||
| </span> | ||
| </span> | ||
| </header> | ||
| <section class="temperature__section"> | ||
| <h2>Actual Temperature</h2> | ||
| <div class="temperature__content"> | ||
| </div> | ||
| <span id="actualValue" class="orange">65°</span> | ||
| </div> | ||
| </section> | ||
| <section class="sky__section"> | ||
| <h2>Desired Sky</h2> | ||
| <select id="skySelect"> | ||
| <option value="sunny">Sunny</option> | ||
| <option value="cloudy">Cloudy</option> | ||
| <option value="rainy">Rainy</option> | ||
| <option value="snowy">Snowy</option> | ||
| </select> | ||
| </section> | ||
| <section class="garden__section"> |
| <h1> Sky & Garden </h1> | ||
| <div id="gardenContent" class="garden__content sunny"> | ||
| <span id="desiredSky">☀️🌞☀️🌞☀️🌞☀️🌞☀️🌞☀️🌞☀️🌞</span> | ||
| <span id="landscape">🌼🌸🌸🌸🌼🌸🌸🌸🌼🌸🌸🌸🌼🌸🌸🌸🌼</span> | ||
| </div> | ||
| </h2> |
There was a problem hiding this comment.
Rather than hard-coding a value for UI elements (which just happens to match the initial value backing the control), I like to leave the initial markup blank, and update the UI on first load from the initial backing values in the JS code. So like here, leave off the emojis, then when the document loads, update the UI in the JS to show the initial sky and landscape value. This would protect us from changing the default starting JS value, but having it be out of sync with what the UI is showing.
I would tend to do this for any of the configurable elements (temperature, ground, sky, city).
| <span id="increaseTempControl">⬆️</span> | ||
| <span id="decreaseTempControl">⬇️</span> | ||
| </div> | ||
| <span id="tempValue">70°</span> |
There was a problem hiding this comment.
Rather than hard-coding a value for UI elements (which just happens to match the initial value backing the control), I like to leave the initial markup blank, and update the UI on first load from the initial backing values in the JS code. So like here, leave off the 70, then when the document loads, update the UI in the JS to show the initial temperature value. This would protect us from changing the default starting JS value, but having it be out of sync with what the UI is showing.
I would tend to do this for any of the configurable elements (temperature, ground, sky, city).
|
|
||
| const state = { | ||
| temp : 70, | ||
| sky : document.getElementById('skySelect'), |
There was a problem hiding this comment.
Instead of having a hard coded default sky state, how might you refactor this so the logic is more data driven? The sky should really be displayed based on the call to the api
| const state = { | ||
| temp : 70, | ||
| sky : document.getElementById('skySelect'), | ||
| location : '', |
There was a problem hiding this comment.
The few values in your default state could be the lat, long, temp and city name
| // TEMP CHANGES | ||
| const increaseTemp = () => { | ||
| state.temp ++; | ||
| tempValue.innerHTML = state.temp + "°" ; |
There was a problem hiding this comment.
Instead of using the + symbol here you could use concatenation ${state.temp}°
| .then((response) => { | ||
| state.lon = response.data[0].lon | ||
| state.lat = response.data[0].lat | ||
| }) | ||
| .then(()=> { | ||
| getWeather(); |
There was a problem hiding this comment.
You can combine these two .thens into one like so:
.then((response) => {
state.lat = response.data[0].lat;
state.long = response.data[0].lon;
getWeather();
})
| tempValue.innerHTML = state.temp + "°" ; | ||
| actualTempValue.innerHTML = state.temp + "°" ; |
There was a problem hiding this comment.
We should separate our concerns here by having another function handle updating our UI
| const landscapeChange = () =>{ | ||
| if (state.temp >= 80){ | ||
| desiredLandscape.textContent = "🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂" | ||
| bgColor.style.backgroundColor = '#E24E1B' | ||
| } | ||
| else if (state.temp >= 70 ){ | ||
| desiredLandscape.textContent = "🌸🌿🌼__🌷🌻🌿_🌱_🌻🌷" | ||
| bgColor.style.backgroundColor = '#E5B25D' | ||
| } | ||
| else if (state.temp >= 60 ){ | ||
| desiredLandscape.textContent = "🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃" | ||
| bgColor.style.backgroundColor = '#D0CFEC' | ||
| } | ||
| else if (state.temp >= 50 ){ | ||
| desiredLandscape.textContent = "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲" | ||
| bgColor.style.backgroundColor = "#875C74" | ||
| } | ||
| else if (state.temp <= 40 ){ | ||
| bgColor.style.backgroundColor = "#1B4079" | ||
| } | ||
| } |
There was a problem hiding this comment.
There’s an issue with wave 02 (temperature), the requirements specify that the temperature ranges should change the text color or the background of the temperature & the landscape depending on what temperature it is. Currently your code is updating the background color of the entire page.
| const changeCityName = () => { | ||
| headerCityName.innerHTML = cityNameInput.value | ||
|
|
||
| } |
There was a problem hiding this comment.
There's an issue with wave 03 (naming the city), the requirements specify that there should be an element that displays a city name & a text input element that allows the user to change the city name. The city name must also be updated every time there's a change to the text input element. Currently your code is not displaying the city name when you reset the city.
| const resetLocation = () => { | ||
| state.location = ''; | ||
| cityNameInput.value = state.location; | ||
| changeCityName(); | ||
| } |
There was a problem hiding this comment.
When you reset the city it should be updated to a selected city instead of an empty string like so:
cityNameInput.value = 'Seattle';
There was a problem hiding this comment.
Great work on this project Ryan! The code you wrote for wave 2 & 3 is really close, I've put in a suggested solution, but I didn't have time to test it so it may need some adjustments. The code in this project is really clean and easy to read. This project is yellow 🟡
No description provided.