Sharks - Camilla #80
Sharks - Camilla #80
Conversation
| <title>Weather Report</title> | ||
| </head> | ||
| <body> | ||
|
|
||
| <main> |
There was a problem hiding this comment.
👍 Your HTML is semantic and clearly written. Nice job!
| const tempCounterContainer = document.querySelector('#tempNumber'); | ||
| tempCounterContainer.textContent = state.tempCounter +'°'; | ||
| let tempCircle = document.getElementById('circle'); | ||
| tempCircle = displayColorAndGarden(event); |
There was a problem hiding this comment.
Notice that lines 18-21 are identical to lines 10-13.
You can put these 4 lines into a helper method like updateTempAndDisplay() and then call that helper method in the increase/decreaseTemp methods.
| if (state.tempCounter >= 80) { | ||
| tempCircleColor.style.color = 'red'; | ||
| gardenContainer.textContent = '🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂'; | ||
| } else if (state.tempCounter <= 79 && state.tempCounter >= 70) { | ||
| tempCircleColor.style.color = 'orange'; | ||
| gardenContainer.textContent = '🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷'; | ||
| } else if (state.tempCounter <= 69 && state.tempCounter >= 60) { | ||
| tempCircleColor.style.color = 'yellow'; | ||
| gardenContainer.textContent = '🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃'; | ||
| } else if (state.tempCounter <= 59 && state.tempCounter >= 50) { | ||
| tempCircleColor.style.color = 'aquamarine'; | ||
| gardenContainer.textContent = '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲'; | ||
| } else if (state.tempCounter <= 49 && state.tempCounter >= 30) { | ||
| tempCircleColor.style.color = 'teal'; | ||
| gardenContainer.textContent = '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲'; | ||
| } |
There was a problem hiding this comment.
I like that you combined the color and garden data in this method.
Check out the logic in this method, see how similar the branches are of the conditional statement. We look for whether the temperature is within a range, and pick a color/garden accordingly. What if we had a list of records that we could iterate through to find the values. We could set up something like:
const TEMP_COLORS_GARDENS = [
{ upperBound: 49, color: "teal", garden: '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲'},
{ upperBound: 59, color: "green", garden: '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲'},
{ upperBound: 69, color: "yellow", garden: '🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃'},
{ upperBound: 79, color: "orange", garden: '🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷'},
{ upperBound: 80, color: "red", garden: '🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂'}
];Then your method would iterate through TEMP_COLORS_GARDENTS and find the first record that has an upper bound higher than state.tempCounter, then use it as the source of picking the color which would help make the function a bit more concise.
This could accommodate a scenario where you might have even more color and garden combinations in the future, which would prevent your conditional statement from being a really long block of if/elif/elif and so on
| if (newOption === 'sunny') { | ||
| changeSkyEl.textContent = '☁️ ☁️ ☁️ ☀️ ☁️ ☁️' | ||
| } else if (newOption === 'cloudy') { | ||
| changeSkyEl.textContent = '☁️☁️ ☁️ ☁️☁️ 🌤 ☁️ ☁️☁️' | ||
| } else if (newOption === 'rainy') { | ||
| changeSkyEl.textContent = '🌧🌈⛈🌧🌧💧⛈🌧🌦🌧💧🌧🌧'; | ||
| } else if (newOption === 'snowy') { | ||
| changeSkyEl.textContent = '🌨❄️🌨🌨❄️❄️🌨❄️🌨❄️❄️🌨🌨' | ||
| } |
There was a problem hiding this comment.
Could also put these sky options in an objefct and iterate through it like what I suggested in displayColorAndGarden
|
|
||
| function resetCity() { | ||
| const cityInput = document.querySelector('input'); | ||
| cityInput.value = 'Kąśna Dolna, Poland'; |
There was a problem hiding this comment.
Consider creating a constant like DEFAULT_CITY for 'Kąśna Dolna, Poland' instead of using a string literal here. It's good practice to use constants when the literal value won't change and also if someone reads
cityInput.value = DEFAULT_CITY;the intent is a little more clear.
| const registerEventHandlers = (event) => { | ||
| const tempButtonUp = document.getElementById('tempButtonUp'); | ||
| tempButtonUp.addEventListener('click', increaseTemp); | ||
|
|
||
| const tempButtonDown = document.querySelector('#tempButtonDown'); | ||
| tempButtonDown.addEventListener('click', decreaseTemp); | ||
|
|
||
| const cityInput = document.querySelector('input'); | ||
| const changeHeading = document.getElementById('miniheader'); | ||
| cityInput.addEventListener('input', changeCity); | ||
|
|
||
| const getRealTempButton = document.getElementById('getRealTemp'); | ||
| getRealTempButton.addEventListener('click', getLatAndLon); | ||
|
|
||
| const skySelector = document.querySelector('#skySelect'); | ||
| skySelector.addEventListener('change', displaySky); | ||
|
|
||
| const resetButton = document.querySelector('#resetButton'); | ||
| resetButton.addEventListener('click', resetCity); | ||
|
|
||
| }; | ||
|
|
||
| document.addEventListener('DOMContentLoaded', registerEventHandlers); |
There was a problem hiding this comment.
For readability sake, I would put lines 73 - 95 at the bottom of the file.
| const fromKelvinToFahrenheit = function(temp) { | ||
| return (temp - 273.15) * 9/5 + 32; | ||
| }; |
There was a problem hiding this comment.
Nice, I like that this is in a helper method even though it's short. Helps with readability in your getWeather() method
| weatherResponse = response.data; | ||
| let realTemp = weatherResponse.current.temp | ||
| realTemp = Math.floor(fromKelvinToFahrenheit(realTemp)); |
There was a problem hiding this comment.
Consider putting this logic in a helper method to make your .then block more concise and readable, something like formatRealTemp()
| let tempNumElement = document.getElementById('tempNumber'); | ||
| tempNumElement.textContent = realTemp; | ||
| state.tempCounter = realTemp; | ||
| displayColorAndGarden(event); |
There was a problem hiding this comment.
Consider putting this logic in a helper method, something like updateRealTemp()
No description provided.