Ashley_Benson_Seaturtles #19
Conversation
There was a problem hiding this comment.
Nice job!
Remember that we only need to make a venv for Python projects, and it should be listed in our .gitignore file.
Storing app state in variables is a useful code strategy (get your city name in there too!). Structuring our code so that there are separate functions for updating those variables, and then publishing the values helps us manage user interactions and data processing. Think about how to restructure the axios calls to be a bit more generally reusable (how can we avoid making the second call directly from the first?). And keep an eye out for areas where we can use data structures to reduce code duplication.
| <div class="input_container"> | ||
| <i class="fa-thin fa-magnifying-glass"></i> | ||
| <input class="input_field" type="text" placeholder="Search City" name="city search" /> | ||
| </div> |
There was a problem hiding this comment.
It doesn't look like this field is used for the realtime city. The realtime lookup uses #cityNameInput so this input could be removed.
| @@ -1,12 +1,67 @@ | |||
| <!DOCTYPE html> | |||
| <html lang="en"> | |||
|
|
|||
There was a problem hiding this comment.
This isn't a Python project, so we didn't need to create a venv. If this were a Python project, we'd want to make sure that vevn was listed in the .gitignore, since the venv should not get checked in.
| <h1>Ashley's Weather Report</h1> | ||
| <span class="subheader"> | ||
| <span id="headerCityName" class="header_city"></span> | ||
| </span> |
There was a problem hiding this comment.
Good choice to not use h2 as the subheading. h2 is a heading in its own right, and isn't intended to just be the subtitle to an h1 heading.
| } | ||
|
|
||
| .input_field { | ||
| font-family: Cambria, Cochin, Georgia, Times, 'Times New Roman', serif; |
There was a problem hiding this comment.
I'm a little surprised that you needed to set the font-family in so many classes. font-family is an inherited property so in theory shouldn't need to be set so frequently. If it's not "taking", some alternatives would be to make a class that sets the app font, and apply that class to any elements that need to use the app font (in addition to any other classes on the element). Alternatively, you could look into CSS vars, which would allow you to define the list of fonts once, then refer to that list in all the rules that need it.
| @@ -0,0 +1,116 @@ | |||
| "use strict"; | |||
There was a problem hiding this comment.
Remember to make git commits as you work. The README asked us to make at least 5 commits (1 per wave is a good start).
|
|
||
| const getRealTimeTemp = () => { | ||
| const cityName = document.getElementById("cityNameInput").value; | ||
| axios.get("http://127.0.0.1:5000/location", { params: { q: cityName } }).then( |
There was a problem hiding this comment.
Consider storing the unchanging part of the URL (the base URL) in a variable elsewhere, then use string interpolation to build the full path here. This would make it easier to change where all of the endpoints in the app are going by changing a single variable.
| axios.get("http://127.0.0.1:5000/weather", { params: { lat: lat1, lon: lon1 } }).then( | ||
| (response) => { | ||
| const realTemp = response.data.current.temp | ||
| const farTemp = Math.round((realTemp - 273.15) * 1.8000 + 32.00) |
There was a problem hiding this comment.
Consider moving this conversion code into a helper function.
| (response) => { | ||
| const lat1 = response.data[0].lat | ||
| const lon1 = response.data[0].lon | ||
| axios.get("http://127.0.0.1:5000/weather", { params: { lat: lat1, lon: lon1 } }).then( |
There was a problem hiding this comment.
Nesting another axios call in the response handler works out OK in this situation since the two calls we're making are pretty small and related, so it makes sense to have them grouped together under the getRealTimeTemp function. But it can also be useful to make these two calls into individual helpers, and then combining them in another function. Something like (very loosely):
const getLatLon = (...) => { return axios.get(...) };
const getTemp = (...) => { return axios.get(...) };
const getTempForCity = (...) => {
return getLatLon(...)
.then(response => getTemp(response.data.latLonData))
.then(response => publishTempData(response.data.tempData))
.catch(err => console.log(err))
};| const skyChanger = () => { | ||
| const skyContainer = document.getElementById("sky"); | ||
|
|
||
| if (document.getElementById('skySelect').value === 'sun') { |
There was a problem hiding this comment.
Like for the temp check each of these clauses is very repetitive. We look for a matching value to locate information about what values to set in the UI. Consider using object (key) lookups to make this more data-driven. If the values are stored elsewhere (maybe loaded from an api) then the code here would essentially reduce down to a key lookup in an object!
| headerCityContainer.textContent = getCityName; | ||
| }; | ||
| // reset to default city name | ||
| const resetCity = () => { |
There was a problem hiding this comment.
👍 Nice reuse of your update function.
No description provided.