-
Notifications
You must be signed in to change notification settings - Fork 527
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
PR to review Jackie's weather-report submission #31
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,130 @@ | ||
import 'regenerator-runtime/runtime'; | ||
import axios from 'axios'; | ||
|
||
const state = { | ||
temperature: 40 | ||
}; | ||
|
||
const tempText = document.querySelector("#tempValue"); | ||
const landscapeText = document.querySelector("#landscape"); | ||
const cityText = document.querySelector("#cityNameInput"); | ||
const skyText = document.querySelector("#sky"); | ||
|
||
const changeSky = (sky) => { | ||
if (sky === "sunny") { | ||
skyText.innerHTML = "☁️ ☁️ ☁️ ☀️ ☁️ ☁️" | ||
} else if (sky === "cloudy") { | ||
skyText.innerHTML = "☁️☁️ ☁️ ☁️☁️ ☁️ 🌤 ☁️ ☁️☁️" | ||
} else if (sky === "rainy") { | ||
skyText.innerHTML = "🌧🌈⛈🌧🌧💧⛈🌧🌦🌧💧🌧🌧" | ||
} else if (sky === "snowy") { | ||
skyText.innerHTML = "🌨❄️🌨🌨❄️❄️🌨❄️🌨❄️❄️🌨🌨" | ||
} | ||
Comment on lines
+14
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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": "☁️☁️ ☁️ ☁️☁️ ☁️ 🌤 ☁️ ☁️☁️"} // etc When 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 |
||
}; | ||
|
||
const changeSkyValue = () => { | ||
const skyValue = document.getElementById('skySelect').value; | ||
changeSky(skyValue); | ||
}; | ||
|
||
const increaseTemp = () => { | ||
state.temperature += 1; | ||
updateTempUi(); | ||
}; | ||
Comment on lines
+30
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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"; | ||
} | ||
}; | ||
|
||
Comment on lines
+37
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here about using an object to store these values (see above in You could have an object like:
|
||
const decreaseTemp = () => { | ||
state.temperature -= 1; | ||
updateTempUi(); | ||
}; | ||
|
||
const updateCityHeader = () => { | ||
let currentCityInput = cityText.value; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this variable isn't reassigned we should use const instead |
||
const cityHeader = document.querySelector("#headerCityName"); | ||
cityHeader.innerHTML = `${currentCityInput}`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can directly access the variable currentCityInput without string templates here. |
||
}; | ||
|
||
const resetCityName = () => { | ||
cityText.value = 'Seattle' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer to use a descriptively named variable to reference this value instead of a string literal. Something like: const DEFAULT_CITY = 'Seattle';
cityText.value = DEFAULT_CITY; |
||
updateCityHeader(); | ||
}; | ||
|
||
const updateCurrentTemp = () => { | ||
findLatitudeAndLongitude(cityText.value); | ||
}; | ||
|
||
const findLatitudeAndLongitude = (cityName) => { | ||
axios.get('https://weather-proxy-s44a.onrender.com/location', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer to use a variable to reference this variable instead of a string literal. |
||
params: { | ||
q: cityName, | ||
}, | ||
}) | ||
.then((response) => { | ||
const latitude = response.data[0].lat; | ||
const longitude = response.data[0].lon; | ||
findLocationTemp(latitude, longitude) | ||
Comment on lines
+82
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
console.log('success in finding Latitude And Longitude!', latitude, longitude); | ||
}) | ||
.catch((error) => { | ||
console.log('error!', error.response.data); | ||
}); | ||
}; | ||
|
||
const findLocationTemp = (latitude, longitude) => { | ||
axios.get('https://weather-proxy-s44a.onrender.com/weather', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use a constant variable to reference the URL instead of a string literal |
||
params: { | ||
lat: latitude, | ||
lon: longitude | ||
}, | ||
}) | ||
.then((response) => { | ||
const tempInKelvin = response.data.main.temp; | ||
let tempInFahrenheit = (tempInKelvin - 273) * 9/5 + 32; | ||
tempInFahrenheit = Math.floor(tempInFahrenheit); | ||
state.temperature = tempInFahrenheit; | ||
updateTempUi(); | ||
Comment on lines
+100
to
+104
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
console.log(tempInFahrenheit); | ||
}); | ||
}; | ||
|
||
|
||
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); | ||
Comment on lines
+110
to
+130
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
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.
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