Yonese J. - Sharks #63
Conversation
There was a problem hiding this comment.
Nice work on this project!
You made good use of DOM features to handle button clicks related to the temperature changing colors. Your sky selection and landscape features work great!
Let me know if you have any questions about the comments.
🟢 for weather-report!
| <h1 id="navHeaderOne">Welcome to Weather Report!</h1> | ||
| <span id="navHeaderThree">Currently displaying temperature for <span id="cityName">Orlando</span></span> |
There was a problem hiding this comment.
For the scope of a small page like this, having id being called navHeaderOne or navHeaderThree is ok, but it's helpful to have id or class names that is a little more descriptive. In the future when you're working with pages with many elements, your id should clue you in to what element it's on so when you're checking out your css or js that uses navHeaderOne you might not need to go back to the HTML to see what element it's refering to.
For example, navHeaderOne could be something like weatherWelcomeMessage
|
|
||
|
|
||
| <script src="src/index.js"></script> | ||
| <script src="./node_modules/axios/dist/axios.min.js"></script> |
| if(state.temp <= 49){ | ||
| state.tempColor = "teal"; | ||
| temp.style.color = state.tempColor; | ||
| } else if(50 <= state.temp && state.temp <= 59){ | ||
| state.tempColor = "green" | ||
| temp.style.color = state.tempColor; | ||
| } else if(60 <= state.temp && state.temp <= 69){ | ||
| state.tempColor = "yellow"; | ||
| temp.style.color = state.tempColor; | ||
| } else if(70 <= state.temp && state.temp <= 79){ | ||
| state.tempColor = "orange"; | ||
| temp.style.color = state.tempColor; | ||
| } else if(state.temp >= 80){ | ||
| state.tempColor = "red"; | ||
| temp.style.color = state.tempColor; | ||
| } |
There was a problem hiding this comment.
In this method you set state.tempColor but then it's not being accessed anywhere else (other than on first page load on line 14.
You could directly set the color on temp.style.color like:
if(state.temp <= 49){
temp.style.color ="teal";
}|
|
||
| const state = { | ||
| temp : 80, | ||
| tempColor: "red", |
There was a problem hiding this comment.
I see that you add tempColor to state in order to access the value on line 14 to initialize your page so that the 80 degrees shows up in red.
After this though, you don't use tempColor (I dropped a comment tempColor about setting state.color).
Since you don't need to persist tempColor, you could leave it out of the state object
|
|
||
| const temp = document.getElementById("tempValue"); | ||
| temp.textContent = state.temp + "°"; | ||
| temp.style.color = state.tempColor; |
There was a problem hiding this comment.
If you remove tempColor from `state, then you can just hardCode red here
temp.style.color = "red";
| // const getRealTempBtn = document.getElementById("realTempBtn"); | ||
| // getRealTempBtn.addEventListener("click", getRealTemp); |
There was a problem hiding this comment.
Remember to remove unused, commented out code so that it doesn't accidentally get uncommented and incorrectly executes.
| const getRealTemp = () => { | ||
| state.changeCity = document.getElementById("city").value; | ||
| getLatLon(); | ||
| } |
There was a problem hiding this comment.
Is this method being used? I don't think it is. It's referenced in a comment but never executes so you can remove it altogether
| }; | ||
|
|
||
| const getWeather = () => { | ||
| axios.get("http://127.0.01:5000/weather", { |
There was a problem hiding this comment.
You can store this string literal into a variable WEATHER_URL
| }; | ||
|
|
||
| const getLatLon = () => { | ||
| axios.get("http://127.0.01:5000/location", { |
There was a problem hiding this comment.
You can store this url into a constant LOCATION_URL
| const input = document.querySelector("input"); | ||
| const city = document.getElementById("cityName"); | ||
| input.addEventListener("input", () => { | ||
| state.changeCity = document.getElementById("city").value; | ||
| document.getElementById("cityName").innerHTML = state.changeCity; | ||
| }); | ||
|
|
||
| const reset = document.getElementById("resetBtn"); | ||
| const resetValue = input.getAttribute("placeholder"); | ||
| reset.addEventListener("click", () => { | ||
| input.value = resetValue; | ||
| city.textContent = resetValue; | ||
| }) |
There was a problem hiding this comment.
It would make more sense for these statements to be put in registerEventHandlers() instead of sitting out here out in the open.
I see that lines 77-89 execute when the page first renders so it makes sense to have them in registerEventHandlers with all the others
No description provided.