weather-report panthers Tiffany Rogers #90
Conversation
| .catch((error) => { | ||
| alert(error); | ||
| }) | ||
| }; |
There was a problem hiding this comment.
Nice promise chaining for these functions, Tiffany! I have one refactoring advice for you and that is to implement the async &c await functionality of Javascript in order to make your code more concise and readable. doing something like:
const getAPI = async () => {
let lat, long;
try {
const response = await axios.get('http://127.0.0.1:5000/location', {
params: {
q: userSearch.value,
format: 'json',
}})
lat = response.data[0].lat;
long = response.data[0].lon;
getWeather(lat, long)
} catch (err) {
console.log(err);
}
}Using the await keyword lets us stop the global execution for the function it is in and wait for the promise to be fulfilled. This allows us to cut out the promise chaining here and just save return information from the axios calls as variables. We can then use said variables like we ordinarily would in the then chain of the promise. Then we can use a try/catch block for error handling. Finally, I would suggest breaking the location and weather API calls into their own functions so that you can avoid nesting try/blocks and just overall concise code.
| function increment(){ | ||
| temp += 1; | ||
| temperature.textContent=temp | ||
| console.log(temp) |
There was a problem hiding this comment.
Remember to remove your console.logs before pushing into production!
| <input type="text" id="city-search" value="Columbus" /> | ||
| <button | ||
| style=" |
There was a problem hiding this comment.
When doing this much styling it is best to move it inside of your css file so it doesn't congest your html files.
| temperature.style.color="blue" | ||
| }else { | ||
| temperature.style.color="#1100ab" | ||
| } |
There was a problem hiding this comment.
So here you have the right idea for your if statements, however you could just put this here:
if (temp > 60)
| spring.style.display="none" | ||
| fall.style.display="none" | ||
| winter.style.display="flex" | ||
| } |
There was a problem hiding this comment.
So here, the refactor that I would suggest would start in the HTML file. Instead of putting all the landscapes I would just put one of them. Something like : <div id='landscape'></div> After that, I would change the code you have here into something like:
const landscape = document.getElementById('landscape')
if (temp >= 65) {
landscape.innerHTML = 😎S☀️u🌞n🌅n🌞y😎
}This way you can save your fingers from typing a lot more code! I would do the same thing in the dropDownValue function.
| defaultSky.style.display = 'none'; | ||
| }; | ||
|
|
||
| dropdownValue() |
There was a problem hiding this comment.
I see you are invoking the certain functions in order to get the intital data displayed on the first render of the page. I would just move these all somewhere together. Given that you used the function keyword your functions have been hoisted and you can move them anywhere you please.
| resetCityBtn.addEventListener('click', resetCity); | ||
| getWeatherBtn.addEventListener('click', getApi); | ||
| userSearch.addEventListener('input', displayInput); | ||
| skyDropDown.addEventListener('change', dropdownValue); |
There was a problem hiding this comment.
Here the only suggestion is to move these inside a registerHandlers function that is made the callback to theDOMContentLoadedevent handler.
| } | ||
| footer{ | ||
| text-align: center; | ||
| } |
|
Can you please explain the if (temp > 60)?
I don't see how that would work when I was using so many colors to show
temp.
I live in Ohio and we have all four seasons here and they can be dramatic,
so I wanted to show that.
…On Tue, Jan 10, 2023 at 7:53 PM Mikelle ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/index.js
<#90 (comment)>
:
> + temperature.style.color="#ab0000"
+ }else if (temp <= 89 && temp >= 80){
+ temperature.style.color="red"
+ }else if (temp <= 79 && temp >= 70){
+ temperature.style.color="orange"
+ }else if (temp <= 69 && temp >= 60){
+ temperature.style.color="green"
+ }else if (temp <= 59 && temp >= 50){
+ temperature.style.color="#38f9cc"
+ }else if (temp <= 49 && temp >= 40){
+ temperature.style.color="#378aea"
+ }else if (temp <= 39 && temp >= 30){
+ temperature.style.color="blue"
+ }else {
+ temperature.style.color="#1100ab"
+ }
So here you have the right idea for your if statements, however you could
just put this here:
if (temp > 60)
—
Reply to this email directly, view it on GitHub
<#90 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A2NVM47QHBEPFJJEZIYUDT3WRYACDANCNFSM6AAAAAAS52I4LI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Is this personal preference or can it affect code? When I shared my project
in class, I thought the instructor said they liked all the
eventlisteners together at the bottom so they were easy to find.
…On Tue, Jan 10, 2023 at 9:16 PM Mikelle ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/index.js
<#90 (comment)>
:
> + } else if (skyDropDown.value === 'Snowy') {
+ Sunny.style.display = 'none';
+ Cloudy.style.display = 'none';
+ Rainy.style.display = 'none';
+ Snowy.style.display = 'block';
+ defaultSky.style.display = 'none';
+ };
+
+dropdownValue()
+
+upArrow.addEventListener('click', increment);
+downArrow.addEventListener('click', decrement);
+resetCityBtn.addEventListener('click', resetCity);
+getWeatherBtn.addEventListener('click', getApi);
+userSearch.addEventListener('input', displayInput);
+skyDropDown.addEventListener('change', dropdownValue);
Here the only suggestion is to move these inside a registerHandlers
function that is made the callback to theDOMContentLoadedevent handler.
—
Reply to this email directly, view it on GitHub
<#90 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A2NVM45MDDLIQE4A4QZGQLDWRYJWFANCNFSM6AAAAAAS52I4LI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
No description provided.