PR _ Elsje and Bukunmi SL #80
Conversation
There was a problem hiding this comment.
Looks good! Try to think about how to refactor the API call helpers so that they're a little more independent and reusable. I have some suggestions in the comments. There's also quite a bit of code based on similar if/else if statements that we could think about making more data-driven, simplifying our code, and reducing the likelihood of typos, so check my comments for more details.
| --primaryColor: #0066FF; | ||
| --lightColor: #abaed3; |
| grid-template-areas: | ||
| "a b c" | ||
| "a b c" | ||
| "a b c"; |
There was a problem hiding this comment.
These template areas don't appear to be used in the rest of your css. To make use of the template areas, there would need to be corresponding grid-area properties set on various classes that the browser would use to match them up to the appropriate template area.
Instead, it's the grid-template-columns a few lines down that's doing the work here.
| <span id="currentCity">Seattle</span> | ||
| </div> | ||
| <div> | ||
| <h2 class="interactive">To view the precise temp in {city}, click here:</h2> |
There was a problem hiding this comment.
Looks like you intended to fill in the city name in this message.
|
|
||
| <main class="container"> | ||
| <!-- column a --> | ||
| <section class="a"> |
There was a problem hiding this comment.
You don't have a class called a in your styles. I'm assuming this was intended to associate this content with the first column in the grid. It turns out that the grid-template-columns setting simply takes the child elements in order, assigning them to the columns in order.
| <section class="c"> | ||
| <div> | ||
| <h2>Current temperature is:</h2> | ||
| <span id="displayedTemp">72</span> |
There was a problem hiding this comment.
Rather than hard-coding a value for UI elements (which just happens to match the initial value backing the control), I like to leave the initial markup blank, and update the UI on first load from the initial backing values in the JS code. So like here, leave off the 72, then when the document loads, update the UI in the JS to show the initial temperature value. This would protect us from changing the default starting JS value, but having it be out of sync with what the UI is showing.
I would tend to do this for any of the configurable elements (temperature, ground, sky, city).
| lat: state.lat, | ||
| lon: state.lon, |
There was a problem hiding this comment.
The app doesn't need to track the lat/lon over time. It's only used during the chained city/weather call. So rather than stashing those values in the state, consider making the lat and lon input parameters to this function.
| const toFahrenheit = (k) => (k - 273.15) * (9 / 5) + 32; | ||
|
|
||
| const getWeather = () => { | ||
| axios |
There was a problem hiding this comment.
We should always return any promise chains we create in helper functions (axios.get starts a promise chain) so that additional actions could be chained onto the end after the function logic completes.
return axios.get(...).then(...)| state.temp = cityTemp | ||
| console.log("success!!", response.status); | ||
| tempChange(); | ||
| landscapeChange(); |
There was a problem hiding this comment.
Rather than stashing the temperature result directly in the application state here, consider returning the temperature from the .then callback. This would make it available to a subsequent .then callback that could be chained outside this helper. In that case, we also need to be sure to return the end of the promise chain itself from the helper, so that additional actions can be chained to run after this helper completes.
|
|
||
| const getLatAndLon = () => { | ||
| let lat, lon; | ||
| axios |
There was a problem hiding this comment.
Be sure to return here as mentioned above.
| state.lat = lat; | ||
| state.lon = lon; | ||
| getWeather(); |
There was a problem hiding this comment.
Rather than stashing the lat/lon and then calling the weather api call directly, consider returning the lat/lon from this .then callback so that it would be available to act as the input to a subsequent .then callback. We could then make a separate function whose responsibility is to combine the city->lat/lon function with the lat/lon->weather function.
Assuming getLatAndLon was a function taking a city name and returning a promise to an object shaped like {lat: 0, lon: 0}, and getWeather was a function taking lat and lon values, and returning a promise to a temperature, we could write a third function getWeatherForCity as follows:
const getWeatherForCity = (cityName) => {
return getLatAndLon(cityName)
.then(({lat, lon}) => {
return getWeather(lat, lon);
})
};We could then use this as
getWeatherForCity(state.city)
.then(temp => {
// code that was in getWeather above
state.temp = temp;
console.log("success!!", response.status);
tempChange();
landscapeChange();
})
No description provided.