C24 Crow Tatiana and Crow Hok Yin Iris Cheung, Weather Report#9
C24 Crow Tatiana and Crow Hok Yin Iris Cheung, Weather Report#9FluenceMind wants to merge 15 commits into
Conversation
… change and weather garden lanscape change
mikellewade
left a comment
There was a problem hiding this comment.
@FluenceMind and @Iris-Cheung-HY, nice work on your weather report. Thank you for the patience! Even instructors can have a back log!
| longitude = response.data[0].lon; | ||
| console.log('success in findLatitudeAndLongitude!', latitude, longitude); | ||
|
|
||
| return{latitude, longitude}; |
There was a problem hiding this comment.
| return{latitude, longitude}; | |
| return { latitude, longitude }; |
| const axios = window.axios | ||
|
|
||
| export const findLatitudeAndLongitude = (cityName) => { | ||
| let latitude, longitude; |
There was a problem hiding this comment.
Declaring these variable here makes me think that they will be accessed somewhere in addition to the assignment located in your .then chain. Being they are only being declared here, I would suggest you move the declaration/assignment into the .then entirely. This would also be more secure since it would be locally scoped to the callback of the .then.
| let latitude, longitude; |
| export const findLatitudeAndLongitude = (cityName) => { | ||
| let latitude, longitude; | ||
|
|
||
| return axios.get('http://localhost:5000/location', |
There was a problem hiding this comment.
Since you would be using the base of this URL in multiple places, to make your code more D.R.Y by assigning a constant variable to it.
const BASE_URL = 'http://localhost:5000/'Then it could be used in the following manner:
return axios.get(`${BASE_URL}location',| latitude = response.data[0].lat; | ||
| longitude = response.data[0].lon; |
There was a problem hiding this comment.
| latitude = response.data[0].lat; | |
| longitude = response.data[0].lon; | |
| const latitude = response.data[0].lat; | |
| const longitude = response.data[0].lon; |
| .then((response) => { | ||
| latitude = response.data[0].lat; | ||
| longitude = response.data[0].lon; | ||
| console.log('success in findLatitudeAndLongitude!', latitude, longitude); |
There was a problem hiding this comment.
Don't forget to remove your console logs from your production code.
| const resetInputButton = () => { | ||
| state.tempValue = ''; | ||
| state.tempDisplay.textContent = `${state.tempValue}`; | ||
| state.skyDisplay.textContent = ''; | ||
| state.landscapeDisplay.textContent = ''; | ||
| state.cityNameInput.value = 'Seattle'; | ||
| state.headerCityName.textContent = 'Seattle'; | ||
| }; |
| const tempValueColorPatternChange = () => { | ||
| if (state.tempValue >= 80) { | ||
| state.tempDisplay.style.color = 'red'; | ||
| state.landscapeDisplay.textContent = `🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂`; | ||
| } else if (state.tempValue >= 70 && state.tempValue <= 79) { | ||
| state.tempDisplay.style.color = 'orange'; | ||
| state.landscapeDisplay.textContent = `"🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷"`; | ||
| } else if (state.tempValue >= 60 && state.tempValue <= 69) { | ||
| state.tempDisplay.style.color = 'yellow'; | ||
| state.landscapeDisplay.textContent = `"🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃"` ; | ||
| } else if (state.tempValue >= 50 && state.tempValue <= 59) { | ||
| state.tempDisplay.style.color = 'green'; | ||
| state.landscapeDisplay.textContent = `"🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲"`; | ||
| } else if (state.tempValue <= 49) { | ||
| state.tempDisplay.style.color = 'teal'; | ||
| state.landscapeDisplay.textContent = `"🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲"`; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Often when we find ourselves writing a plethora of conditional statements, we can usually make it more concise with a refactor. One possible refactor could be:
To make this a little more concise and avoid the multiple if statements we could use a loop and a list to do something like this instead:
const temperatureRanges = [
{ min: 80, color: "red", landscape: "🌵 🐍 🦂 🌵🌵 🐍 🏜 🦂" },
{ min: 70, color: "orange", landscape: "🌸🌿🌼 🌷🌻🌿 ☘️🌱 🌻🌷" },
{ min: 60, color: "salmon", landscape: "🌾🌾 🍃 🪨 🛤 🌾🌾🌾 🍃" },
{ min: 50, color: "green", landscape: "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲" },
{ min: -Infinity, color: "blue", landscape: "⛄️ ⛄️ ⛄️" }, // Default for temperatures <= 49
];
for (const range of temperatureRanges) {
if (state.currentTemp >= range.min) {
tempValueElement.style.color = range.color;
landscapeElement.textContent = range.landscape;
break; // Stop once the matching range is found
}
}What you gain in this implementation is DRY code and maintainability/scalability.
| const updateSky = () => { | ||
| const skyChoice = state.skySelect.value; | ||
|
|
||
| if (skyChoice === 'Sunny') { | ||
| state.skyDisplay.textContent = '☁️ ☁️ ☁️ ☀️ ☁️ ☁️'; | ||
| } else if (skyChoice === 'Cloudy') { | ||
| state.skyDisplay.textContent = '☁️☁️ ☁️ ☁️☁️ ☁️ 🌤 ☁️ ☁️☁️'; | ||
| } else if (skyChoice === 'Rainy') { | ||
| state.skyDisplay.textContent = '🌧🌈⛈🌧🌧💧⛈🌧🌦🌧💧🌧🌧'; | ||
| } else if (skyChoice === 'Snowy') { | ||
| state.skyDisplay.textContent = '🌨❄️🌨🌨❄️❄️🌨❄️🌨❄️❄️🌨🌨'; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Note the consideration for the updating the temperature color.
| const realTimeWeatherTemp = () => { | ||
| findLatitudeAndLongitude(state.cityNameInput.value) | ||
| .then(({latitude, longitude}) => getWeatherTemp(latitude,longitude)) | ||
| .then(({weatherTemp})=> { | ||
| state.tempValue = weatherTemp; | ||
| state.tempDisplay.textContent = `${state.tempValue}`; | ||
| tempValueColorPatternChange(); | ||
| }).catch((error) => { | ||
| console.log('error'); | ||
| }); | ||
| }; |
| const loadControls = () => { | ||
| state.addOneButton = document.getElementById('increaseTempControl'); | ||
| state.minusOneButton = document.getElementById('decreaseTempControl'); | ||
| state.tempDisplay = document.getElementById('tempValue'); | ||
| state.landscapeDisplay = document.getElementById('landscape'); | ||
| state.cityNameInput = document.getElementById('cityNameInput'); | ||
| state.headerCityName = document.getElementById('headerCityName'); | ||
| state.realTimeButton = document.getElementById('currentTempButton'); | ||
| state.skySelect = document.getElementById('skySelect'); | ||
| state.skyDisplay = document.getElementById('sky'); | ||
| state.resetButton = document.getElementById('cityNameReset'); | ||
| }; | ||
| const registerEvents = () => { | ||
| state.addOneButton.addEventListener('click', addtempButton); | ||
| state.minusOneButton.addEventListener('click', minustempButton); | ||
| state.realTimeButton.addEventListener('click', realTimeWeatherTemp); | ||
| state.resetButton.addEventListener('click', resetInputButton); | ||
| state.cityNameInput.addEventListener('input', updateCityName); | ||
| state.skySelect.addEventListener('change', updateSky); | ||
| }; | ||
|
|
||
| const onLoaded = () => { | ||
| loadControls(); | ||
| registerEvents(); | ||
| updateCityName(); | ||
| updateSky(); | ||
| }; | ||
|
|
||
|
|
||
| onLoaded(); No newline at end of file |
There was a problem hiding this comment.
✅
I would suggest moving the logic of adding event handlers to their respective HTML elements into something like document.addEventListener("DOMContentLoaded", RegisterEventHandlers);. That way we register the event handler functions to run when the DOM content has fully loaded. This ensures all HTML elements and resources are available before attempting to attach event handlers or manipulate the DOM.
No description provided.