-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
C22 Dana Delgado & Rong Jiang #11
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work on weather-report, team!
tempIncrease.addEventListener('click', () => { | ||
currentTemp += 1; | ||
updateTemperature(); | ||
}); | ||
|
||
tempDecrease.addEventListener("click", () => { | ||
currentTemp -= 1; | ||
updateTemperature(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice that the difference between these two sections of code is +=
and -=
. Consider creating a helper function that could handle increasing and decreasing the temperature and registering the callback for both buttons.
const handleTempChange = (event) => {
if (event.target.id === 'increaseTempControl') {
currentTemp += 1;
} else {
currentTemp -= 1;
}
updateTemperature();
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer the logic that gets registered with an event to be written in a helper function and then you pass the function when you add an event listener.
It makes your code more readable and it can be easier to debug.
function updateSky(sky) { | ||
const selectedSky = skySelect.value; | ||
skyDisplay.textContent = skyOptions[selectedSky]; | ||
}; | ||
|
||
skySelect.addEventListener('change', updateSky); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how the logic on lines 61-62 are in a helper function and then you pass the function to addEventListener
(instead of passing all of the logic in as an anonymous function on line 65).
const tempValue = document.getElementById('tempValue'); | ||
const tempIncrease = document.getElementById('increaseTempControl'); | ||
const tempDecrease = document.getElementById('decreaseTempControl'); | ||
const landscape = document.getElementById('landscapeDisplay'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's descriptive and makes your code self-documenting when your variable names also indicate what the element is.
const tempValue = document.getElementById('tempValue'); | |
const tempIncrease = document.getElementById('increaseTempControl'); | |
const tempDecrease = document.getElementById('decreaseTempControl'); | |
const landscape = document.getElementById('landscapeDisplay'); | |
const tempValueDisplay = document.getElementById('tempValue'); | |
const tempIncreaseButton = document.getElementById('increaseTempControl'); | |
const tempDecreaseButton = document.getElementById('decreaseTempControl'); | |
const landscapeDisplay= document.getElementById('landscapeDisplay'); |
let currentTemp = 70; | ||
let currentLandscape = ''; | ||
|
||
function updateTemperature() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recall that we can also use minimized syntax that comes from ES6 with arrow functions syntax. For our curriculum, we prefer the arrow syntax.
const updateTemperature = () => {
// your logic here
}
if (currentTemp >= 80) { | ||
tempValue.classList.add("red"); | ||
currentLandscape = "🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂"; | ||
} else if (currentTemp >= 70) { | ||
tempValue.classList.add("orange"); | ||
currentLandscape = "🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷"; | ||
} else if (currentTemp >= 60) { | ||
tempValue.classList.add('yellow'); | ||
currentLandscape = '🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃'; | ||
} else if (currentTemp >= 50) { | ||
tempValue.classList.add("green"); | ||
currentLandscape = "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲"; | ||
} else if (currentTemp <= 49) { | ||
tempValue.classList.add("teal"); | ||
currentLandscape = "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like you use a data structure for skies
, consider using a data structure here to store this info because a bunch of conditional logic like if/else if/else if/else, etc can be brittle and bug-prone.
A list could work here, then you can iterate over the list and check tempVaue
against the first element of the list and get the color and landscape values.
temperatureColorsAndLandscapes = [
[80, "red", "🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂"],
[70, "orange", "🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷"],
[60, "yellow", "🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃"],
[50, "green", "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲"],
[40, "teal", "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲"],
]
// Helper function to fetch location data | ||
function getLocationData(cityName) { | ||
return axios | ||
.get(`http://localhost:5000/location`, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer to refer to this url string with a constant variable. When possible refer to constant values like this with a variable name to make your code more self documenting and help other devs understand what's going on with your code.
const BASE_URL = 'http://localhost:5000';
return axios
.get(`${baseURL}/location`)
getLocationData(cityName) | ||
.then((locationData) => { | ||
const { lat, lon } = locationData; | ||
return axios.get(`http://localhost:5000/weather`, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference this string with a constant variable
const skySelect = document.getElementById('skySelect'); | ||
const skyDisplay = document.getElementById('skyDisplay'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider organizing your file so that all the elements are in one section. I'd move lines 50-51 up to the top with lines 2-5.
if (!Array.isArray(locationResponse.data) || locationResponse.data.length === 0) { | ||
throw new Error("No valid location data found."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer to use a catch
block for error handling instead of manually throwing an error.
.catch((error) => { | ||
alert("Could not fetch weather data. Please try again."); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job using a catch
block here.
No description provided.