C22 Sphinx - Aleida Vieyra + Vlada Rapaport #21
Conversation
…o be completed with weather update
| </head> | ||
|
|
There was a problem hiding this comment.
The spacing is being used for readability and separating different parts of the page, this is fine for me. Just be mindful that some developers might prefer for there to be no empty spaces.
| const updateTemperatureDisplay = () => { | ||
| const tempValueElement = document.getElementById("tempValue"); | ||
| const landscapeElement = document.getElementById("landscape"); | ||
| tempValueElement.textContent = `${state.currentTemp}°`; |
There was a problem hiding this comment.
I want to say that this could just be of the type Number and it would still work so the `` wouldn't be necessary.
| if (state.currentTemp >= 80) { | ||
| tempValueElement.style.color = "red"; | ||
| landscapeElement.textContent = "🌵 🐍 🦂 🌵🌵 🐍 🏜 🦂"; | ||
| } else if (state.currentTemp >= 70) { | ||
| tempValueElement.style.color = "orange"; | ||
| landscapeElement.textContent = "🌸🌿🌼 🌷🌻🌿 ☘️🌱 🌻🌷"; | ||
| } else if (state.currentTemp >= 60) { | ||
| tempValueElement.style.color = "salmon"; | ||
| landscapeElement.textContent = "🌾🌾 🍃 🪨 🛤 🌾🌾🌾 🍃"; | ||
| } else if (state.currentTemp >= 50) { | ||
| tempValueElement.style.color = "green"; | ||
| landscapeElement.textContent = "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲"; | ||
| } else if (state.currentTemp <= 49) { | ||
| tempValueElement.style.color = "blue"; | ||
| landscapeElement.textContent = "⛄️ ⛄️ ⛄️"; | ||
| } |
There was a problem hiding this comment.
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
}
}You do, however, lose some readability. Mainly because the person would need to realize that it utilizes the order retention of a list. Which one do you think you gravitate to and why?
| const increaseTemp = () => { | ||
| state.currentTemp += 1; | ||
| updateTemperatureDisplay(); | ||
| }; | ||
|
|
||
| // Function to decrease temperature | ||
| const decreaseTemp = () => { | ||
| state.currentTemp -= 1; | ||
| updateTemperatureDisplay(); | ||
| }; |
| const temperatureControls = () => { | ||
| const warmUpButton = document.getElementById("increaseTempControl"); | ||
| const coolDownButton = document.getElementById("decreaseTempControl"); | ||
|
|
||
| warmUpButton.addEventListener("click", increaseTemp); | ||
| coolDownButton.addEventListener("click", decreaseTemp); | ||
| }; |
There was a problem hiding this comment.
Straightforward and correct, no notes!
| console.log( | ||
| `Location: ${location}, Latitude: ${latitude}, Longitude: ${longitude}` | ||
| ); |
There was a problem hiding this comment.
Don't forget to remove console logs like this, you could accidentally leak private data. Also just helps to keep the code clutter free.
| return axios | ||
| .get("http://127.0.0.1:5000/location", { | ||
| params: { | ||
| q: location, | ||
| }, | ||
| }) | ||
| .then((response) => { | ||
| let latitude = response.data[0].lat; | ||
| let longitude = response.data[0].lon; | ||
|
|
||
| console.log( | ||
| `Location: ${location}, Latitude: ${latitude}, Longitude: ${longitude}` | ||
| ); | ||
| return { | ||
| latitude: latitude, | ||
| longitude: longitude, | ||
| }; | ||
| }) |
There was a problem hiding this comment.
Great work on this promise! Very easy to follow! Especially since you also decided to use different helper functions to tie things together!
| if (!coordinatesResponse) { | ||
| throw new Error("Coordinates could not be retrieved."); |
There was a problem hiding this comment.
You could also throw this error in the catch above so that the error doesn't propagate further down like it does here.
| getCurrentCityWeather().then((tempKelvin) => { | ||
| if (tempKelvin) { | ||
| const conversion = (tempKelvin - 273) * (9 / 5) + 32; | ||
| state.currentTemp = Math.round(conversion); | ||
| updateTemperatureDisplay(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Nice chaining! Only suggestion, moving the temperature conversion logic into its own function so that it would be more readable since we are already working inside of a .then chain.
| const state = { | ||
| currentTemp: 0, | ||
| cityName: "Miami", | ||
| defaultCityName: "Seattle", | ||
| }; |
There was a problem hiding this comment.
Nice work on selecting what pieces of data should go in state! I think that these a the quintessential choices! ✨
No description provided.