C22 - Phoenix - Luqi - Sphinx - Lorraine #8
Conversation
shiyinxi
changed the title
| document.addEventListener("DOMContentLoaded", registerEventHandlers); | ||
| document.addEventListener("DOMContentLoaded", resetCityName); |
There was a problem hiding this comment.
We can register multiple things to happen in response to an event, but typically, we'll just make a single function that calls the several things we'd like to do. So here, we could make a function setupPage that itself calls registerEventHandlers and resetCityName.
| const cityName = document.getElementById("cityNameInput").value; | ||
| const headerCityName = document.getElementById("headerCityName"); |
There was a problem hiding this comment.
Rather than re-getting the elements I need each time a callback gets run, I like to fetch everything once one the page loads and store the references in some globals or in the state structure. That simplifies the code in the event callbacks a bit, and removes the chance of a typo in the selector leading to problems.
| } | ||
|
|
||
| const increaseTemp = () => { | ||
| const temp = document.getElementById("tempValue"); |
There was a problem hiding this comment.
Rather than re-getting the elements I need each time a callback gets run, I like to fetch everything once one the page loads and store the references in some globals or in the state structure. That simplifies the code in the venet callbacks a bit, and removes the chance of a typo in the selector leading to problems.
| const temp = document.getElementById("tempValue"); | ||
| state.curTemp += 1; | ||
| temp.textContent = state.curTemp; | ||
| changeColorAndLandscape(state.curTemp); |
There was a problem hiding this comment.
Consider restructuring this function (and the decrease) so that all this does is modify the value of state.curTemp and then call another function to update any UI that depends on the current temperature value. Here, that would mostly mean combining the textContent logic, and the call to changeColorAndLandscape into a single helper (maybe publishTemperatureValue). This could be called in the increase/decrease, as well as after the live weather lookup rather than needing to remember to do multiple things in all those places.
| changeColorAndLandscape(state.curTemp); | ||
| }; | ||
|
|
||
| const changeColorAndLandscape = (temp) => { |
There was a problem hiding this comment.
Rather than having a function that directly modifies color and landscape elements, consider restructuring this into two functions, one that determines the color and lanscape to use based on the temperature, and another that actuall applies those values to the UI. Note that currently, each branch has a lot of repetition in the stpes it's doing, with the only difference being the values used
| const findLatitudeAndLongitude = () => { | ||
| let latitude, longitude; | ||
| const cityName = document.getElementById("headerCityName").textContent; | ||
| axios.get('http://127.0.0.1:5000/location', |
There was a problem hiding this comment.
Be sure to return the promise chain. The only way to setup any code that needs to wait for this to finishbefore it runs is to get access to the promise chain we build here, and then .then chain onto the end of it. Never leave a promise chain "trapped" inside a helper method. Always return it.
| } | ||
|
|
||
| const findWeather = (latitude, longitude) => { | ||
| axios.get('http://127.0.0.1:5000/weather', { |
There was a problem hiding this comment.
Here again we would want to return the promise chain. This would allow us to chain additional logic after the API call completes outside this function, rather than having this function need to be responsible both for making the API call and updating the UI. This function should only make the API call, and code elsewhere should chain onto the end of this promise chain with .then to add the logic that updates the UI.
| }) | ||
| .then((response) => { | ||
| const kelvinTemp = response.data.main.temp; | ||
| const fahrenheitTemp = Math.round(kelvinTemp * 9 / 5 - 459.67); |
There was a problem hiding this comment.
Consider moving the kelvin to fahrenheit conversion logic to a helper function.
| const headerCityName = document.getElementById("headerCityName") | ||
| cityName.value = defaultCity; | ||
| headerCityName.textContent = defaultCity; | ||
| findLatitudeAndLongitude(defaultCity); |
There was a problem hiding this comment.
There was no requirement to update the realtime data when resetting the city, but it's easy to do since we have a helper function to handle that. Still, it's a little surprising that it does happen here, when it doesn't happen as we type the city name (we wait until the Get Realtime Temperature button is clicked).
| const changeCityName = () => { | ||
| const cityName = document.getElementById("cityNameInput").value; | ||
| const headerCityName = document.getElementById("headerCityName"); | ||
| headerCityName.textContent = cityName; |
There was a problem hiding this comment.
Simliar to my suggestion for updating values based on the temperature with a helper function, we could make a helper for updating the city name in the UI as well. Even though it's only a single step, it would still be more self-documenting to have a few function calls to something like publishCityName rather than scattered calls to set this textContent. And if we made a change in what happened when publishing the city name, we would then only need to update it in that one function rather than find every place where we updated the textContent directly.
No description provided.