PR to review Jackie's weather-report submission#31
PR to review Jackie's weather-report submission#31yangashley wants to merge 1 commit intoAdaGold:mainfrom
Conversation
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Nice work on weather-report! Let me know if you have questions about my review comments
| import 'regenerator-runtime/runtime'; | ||
| import axios from 'axios'; |
There was a problem hiding this comment.
These were necessary for deployment, but when these imports run locally it breaks the site. When you have conditional code needed for one environment, but not another, you can create a separate branch for deployment that has these specific values and deploy the separate branch. That way you have a main branch that works locally and a deployment branch that has everything it needs to get deployed
| if (sky === "sunny") { | ||
| skyText.innerHTML = "☁️ ☁️ ☁️ ☀️ ☁️ ☁️" | ||
| } else if (sky === "cloudy") { | ||
| skyText.innerHTML = "☁️☁️ ☁️ ☁️☁️ ☁️ 🌤 ☁️ ☁️☁️" | ||
| } else if (sky === "rainy") { | ||
| skyText.innerHTML = "🌧🌈⛈🌧🌧💧⛈🌧🌦🌧💧🌧🌧" | ||
| } else if (sky === "snowy") { | ||
| skyText.innerHTML = "🌨❄️🌨🌨❄️❄️🌨❄️🌨❄️❄️🌨🌨" | ||
| } |
There was a problem hiding this comment.
Another way to format this method would be to have an object that has sky options as keys and and emojis as values, like:
const skyObject = {"sunny": "☁️ ☁️ ☁️ ☀️ ☁️ ☁️", "cloudy": "☁️☁️ ☁️ ☁️☁️ ☁️ 🌤 ☁️ ☁️☁️"} // etcWhen 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 sky and then get the correct values to set the sky
| const increaseTemp = () => { | ||
| state.temperature += 1; | ||
| updateTempUi(); | ||
| }; |
There was a problem hiding this comment.
Notice that this method does essentially the same thing as decreaseTemp but the difference is -= 1 or += 1 (also in JS you can use ++ or -- which is short hand for doing the same thing).
How could you combine the logic of these two methods into one event handler called handleTempChange()?
What if you pass in the event object as an argument and you check if the event's target has an id increaseTempControl or decreaseTempControl and depending on the id then you could ++ or --
const handleTempChange = (event) => {
if (event.target.id === 'increaseTempControl') {
state.temperature++;
} else {
state.temperature--;
}
updateTempUi();
};| }; | ||
|
|
||
| const updateTempUi = () => { | ||
| tempText.innerHTML = `${state.temperature}`; |
There was a problem hiding this comment.
You don't need to use string templating here. You'd use string templating for concatenating strings, like:
const baseUrl = "localhost:500";
axios.get(`${baseUrl}/cats`)| if (state.temperature >= 80) { | ||
| landscapeText.innerHTML = "🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂"; | ||
| tempText.style.color = "red"; | ||
| } else if (state.temperature <= 79 && state.temperature > 69) { | ||
| landscapeText.innerHTML = "🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷"; | ||
| tempText.style.color = "orange"; | ||
| } else if (state.temperature <=69 && state.temperature > 59) { | ||
| landscapeText.innerHTML = "🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃"; | ||
| tempText.style.color = "yellow"; | ||
| } else if (state.temperature <=59 && state.temperature > 49) { | ||
| landscapeText.innerHTML = "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲"; | ||
| tempText.style.color = "green"; | ||
| } else if (state.temperature <=49) { | ||
| landscapeText.innerHTML = "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲"; | ||
| tempText.style.color = "teal"; | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
Same comment here about using an object to store these values (see above inchangeSky()).
You could have an object like:
const tempAndLandscapeObject = {
80: ["🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂", "red"],
70: ["🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷", "orange"],
}
Then you could iterate over the object and check the keys with `state.temperature` and get the array that has the two values you need to set a new landscape and temperature color
| const latitude = response.data[0].lat; | ||
| const longitude = response.data[0].lon; | ||
| findLocationTemp(latitude, longitude) |
| }; | ||
|
|
||
| const findLocationTemp = (latitude, longitude) => { | ||
| axios.get('https://weather-proxy-s44a.onrender.com/weather', { |
There was a problem hiding this comment.
Use a constant variable to reference the URL instead of a string literal
| const tempInKelvin = response.data.main.temp; | ||
| let tempInFahrenheit = (tempInKelvin - 273) * 9/5 + 32; | ||
| tempInFahrenheit = Math.floor(tempInFahrenheit); | ||
| state.temperature = tempInFahrenheit; | ||
| updateTempUi(); |
| const registerEventHandlers = () => { | ||
| const increaseTempButton = document.querySelector("#increaseTempControl"); | ||
| increaseTempButton.addEventListener("click", increaseTemp); | ||
|
|
||
| const decreaseTempButton = document.querySelector("#decreaseTempControl"); | ||
| decreaseTempButton.addEventListener("click", decreaseTemp); | ||
|
|
||
| const updateCity = document.querySelector("#cityNameInput"); | ||
| updateCity.addEventListener("input", updateCityHeader); | ||
|
|
||
| const cityReset = document.querySelector("#cityNameReset"); | ||
| cityReset.addEventListener("click", resetCityName); | ||
|
|
||
| const updateTemp = document.querySelector("#currentTempButton"); | ||
| updateTemp.addEventListener("click", updateCurrentTemp); | ||
|
|
||
| const updateSky = document.querySelector("#skySelect"); | ||
| updateSky.addEventListener("change", changeSkyValue); | ||
| }; | ||
|
|
||
| document.addEventListener("DOMContentLoaded", registerEventHandlers); No newline at end of file |
| }; | ||
|
|
||
| const updateCityHeader = () => { | ||
| let currentCityInput = cityText.value; |
There was a problem hiding this comment.
Since this variable isn't reassigned we should use const instead
1 participant
No description provided.