Somy + Brianna - Sphinx #4
Conversation
…ed registerEventHandlers.
git status
There was a problem hiding this comment.
Nice work with your functionality. Biggest note is that you have your backend and frontend in the same repo and committed/pushed it (backend). I would advise against this (as you probably already know via inspo board) since it can lead to a lot of debugs. Also having 1738 files to load is giving github a hard time. 😅
| const state = { | ||
| city: 'Seattle', | ||
| temperature: 20 | ||
| }; |
There was a problem hiding this comment.
Nice selections for state, these two pieces of data are essentially what directs all the other functionality of our page.
| console.log('Latitude:', lat); | ||
| console.log('Longitude:', lon); | ||
|
|
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.
| .then(weatherResponse => { | ||
| const temperatureInKelvin = weatherResponse.data.main.temp; | ||
| const temperatureInFahrenheit = parseInt((temperatureInKelvin - 273.15) * 9 / 5 + 32); | ||
| console.log(`Temperature in Fahrenheit: ${temperatureInFahrenheit}°F`); | ||
|
|
||
| state.temperature = parseFloat(temperatureInFahrenheit, 10); | ||
|
|
||
| tempValue.textContent = `${state.temperature}°F`; | ||
|
|
||
| headerCityName.textContent = cityName; | ||
|
|
||
| temperatureColor(); | ||
| weatherGardenDisplay(); | ||
| }) | ||
| .catch(error => { | ||
| console.error('Error fetching weather data:', error); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
I would suggest putting this logic for finding the temperature in its own function. The functionality you have its perfectly fine. It is more so for readability purposes. When it comes to promise chaining it can real nested/lengthy real quick. You could have make use of the async/await functionality that JS provides us too. Functionality will be the exact same, just easier to read.
| const tempValue = document.getElementById('tempValue') | ||
| const gardenSection = document.querySelector('.garden__section '); | ||
| const landscape = document.getElementById('landscape') | ||
| const cityNameInput = document.getElementById('cityNameInput'); | ||
| const headerCityName = document.getElementById('headerCityName'); | ||
| const currentTemperature = document.getElementById('currentTempButton') | ||
| const skySection = document.querySelector('.sky__section '); | ||
| const skySelect = document.getElementById('skySelect') | ||
| const sky = document.getElementById('sky') | ||
| const cityNameReset = document.getElementById('cityNameReset') | ||
|
|
There was a problem hiding this comment.
Looks like we are doing similar logic in a few places (finding an element by id and then, later in our code, adding an event handler to it for a specific event). Do you think that this could call for a helper function to make our code a bit more DRY?
| const temperatureIncrease = () => { | ||
| state.temperature += 1; | ||
| tempValue.textContent = `${state.temperature}°F`; | ||
| temperatureColor(); | ||
| weatherGardenDisplay(); | ||
| }; | ||
|
|
||
| // Function to decrease the temperature | ||
| const temperatureDecrease = () => { | ||
| state.temperature -= 1; | ||
| tempValue.textContent = `${state.temperature}°F`; | ||
| temperatureColor(); | ||
| weatherGardenDisplay(); | ||
| }; |
| const temperatureColor = () => { | ||
| tempValue.classList.remove("red", "orange", "yellow", "green", "teal"); | ||
|
|
||
| if (state.temperature >= 80) { | ||
| tempValue.classList.add("red"); | ||
| } else if (state.temperature >= 70) { | ||
| tempValue.classList.add("orange"); | ||
| } else if (state.temperature >= 60) { | ||
| tempValue.classList.add("yellow"); | ||
| } else if (state.temperature >= 50) { | ||
| tempValue.classList.add("green"); | ||
| } else { | ||
| tempValue.classList.add("teal"); | ||
| } | ||
| } | ||
|
|
||
| const weatherGardenDisplay = () => { | ||
| gardenSection.classList.remove("sunny", "cloudy", "rainy", "snowy"); | ||
| if (state.temperature >= 80) { | ||
| gardenSection.classList.add("sunny") | ||
| landscape.innerHTML = "🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂" | ||
| } else if (state.temperature >= 70) { | ||
| gardenSection.classList.add("cloudy") | ||
| landscape.innerHTML = "🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷" | ||
| } else if (state.temperature >= 60) { | ||
| gardenSection.classList.add("rainy") | ||
| landscape.innerHTML = "🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃" | ||
| } else { | ||
| gardenSection.classList.add("snowy") | ||
| landscape.innerHTML = "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲" | ||
| } | ||
| } |
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? (You would have to adjust this code to work with your classes)
| const registerEventHandlers = () => { | ||
| //whatever function is updating UI | ||
| const increaseButton = document.getElementById('increaseButton'); | ||
| increaseButton.addEventListener("click",temperatureIncrease); | ||
|
|
||
| const decreaseButton = document.getElementById('decreaseButton'); | ||
| decreaseButton.addEventListener("click",temperatureDecrease); | ||
|
|
||
| cityNameInput.addEventListener('input', updateCityName); | ||
|
|
||
| const currentTemperatureButton = document.getElementById('currentTempButton'); | ||
| currentTemperatureButton.addEventListener("click", getCurrentTemperature) | ||
|
|
||
| skySelect.addEventListener('change', skyGardenDisplay); | ||
|
|
||
| cityNameReset.addEventListener('click', resetCityName); | ||
|
|
||
| }; | ||
|
|
||
| document.addEventListener("DOMContentLoaded", registerEventHandlers); | ||
|
|
There was a problem hiding this comment.
Nice work ensuring that your document was loaded because adding all your functionality!
No description provided.