Sea Turtles Weather Report Tyrah G.#73
Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Nice job!
Storing app state in variables is a useful code strategy (get your city name and sky 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> | ||
| <div class="city"> | ||
| <form action="#"> | ||
| <input class='textBox'type='search' placeholder="Search a City" name="q"> |
There was a problem hiding this comment.
Make sure to leave space between your attributes.
| </div> | ||
| <script src="./node_modules/axios/dist/axios.min.js"></script> | ||
| <script src="https://requirejs.org/docs/release/2.3.5/minified/require.js"></script> | ||
| <script src="src/index.js" type="text/javascript"></script> |
There was a problem hiding this comment.
text/javascript is the default value for script types, and is usually omitted.
| </div> | ||
| </div> | ||
| <script src="./node_modules/axios/dist/axios.min.js"></script> | ||
| <script src="https://requirejs.org/docs/release/2.3.5/minified/require.js"></script> |
There was a problem hiding this comment.
What is this external import being used for? I don't see anything "require-y" in the code.
| //attach event | ||
| upArrow.addEventListener('click', addTemp); | ||
| downArrow.addEventListener('click', subtractTemp); | ||
| searchBtn.addEventListener('click', cityDisplay); |
There was a problem hiding this comment.
We also wanted to have an input handler on the city text box to update the displayed city name in the UI as the user types.
| } | ||
|
|
||
| .landscape{ | ||
| margin-left: 21cm; |
There was a problem hiding this comment.
I have a hard time wrapping my head around "physical" units in CSS. I tend to stick with pixels, percentages, ems, or some combination of them.
| }) | ||
| .then((response) => { | ||
| let temp = response.data.current.temp; | ||
| temp = Math.floor(1.8 * (temp - 273) + 32); |
There was a problem hiding this comment.
Consider moving this conversion code into a helper function.
| let latitude = response.data[0].lat; | ||
| let longitude = response.data[0].lon; | ||
|
|
||
| getWeather(latitude, longitude); |
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. But it can also be useful to make these two calls into individual helpers (get the lat/lon for the city, get the weather for the lat/lon), 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))
};|
|
||
| //change the sky | ||
| const changeSky = () => { | ||
| let skyOptions = document.querySelector('#skyContainer').children; |
There was a problem hiding this comment.
It looks like you went with a similar approach to your landscape changes here. I think that on the whole, I still tend to prefer having the display data defined in the JS, and then have written out to the HTML as needed (greater opportunity to make the display data-driven), but for the selection of skies you have, this works well. It does have the potential to allow richer layouts for they sky than simple text or an image, so I'd be curious to see how far you can push this!
| } | ||
| let sky = document.querySelector('#sky-selector'); | ||
| let selectedSky = sky.options[sky.selectedIndex].text; | ||
| if (selectedSky === 'Sunny') { |
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 (sky value -> element id)!
| getLatandLon(document.querySelector('.textBox').value); | ||
| }; | ||
|
|
||
| const refreshCity = () => { |
2 participants
No description provided.