Emily - HW 04 #3
Conversation
| }) | ||
|
|
||
|
|
||
| function getWeatherByCity (city) { |
There was a problem hiding this comment.
@kcycle nice work using the standard promise syntax here.
|
|
||
|
|
||
| function getWeatherByCity (city) { | ||
| axios.get(`http://api.openweathermap.org/data/2.5/weather?q=${city}&APPID=1c739a9713a0919a3024a089c3b027df`) |
There was a problem hiding this comment.
@kcycle it's recommended that you use https instead of http whenever available. The use of http here may subject you to a CORS error.
| @@ -8,10 +8,30 @@ | |||
|
|
|||
| <body> | |||
|
|
|||
| <header> | |||
There was a problem hiding this comment.
@kcycle nice updates to the UI
| axios.get(`http://api.openweathermap.org/data/2.5/weather?q=${city}&APPID=1c739a9713a0919a3024a089c3b027df`) | ||
| .then((response) => { | ||
| console.log(response.data) | ||
| displayCityName(response.data.name) |
There was a problem hiding this comment.
@kcycle nice! I like the use of separate functions to display each element. Putting all the display logic into one function would have also adhered to the SRP rule as well.
| return parseInt((kelvinUnits-273.15)*(9/5)+32) | ||
| } | ||
|
|
||
| function setTempFontColor (temperature) { |
There was a problem hiding this comment.
@kcycle great job breaking out this logic into a separate function.
the only suggestion here would be to use if..elseif..esle logic instead of two separate if statements. Take a look at the approach used in the solution: https://github.com/jsd20191008/weather-api-app-solution/blob/master/js/main-promises.js#L82
| $('#max-temp').html('') | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
@kcycle be careful regarding unnecessary blank lines...will not result in an error, but it makes the code harder to scan.
| `<h4>Today's High:</h4> <p>${temperature} F°</p>`) | ||
| } | ||
|
|
||
| function convertToFahrenheit (kelvinUnits) { |
There was a problem hiding this comment.
@kcycle nice job creating a separate function to convert from Celcius to Fahrenheit. However, OpenWeatherMap's API does this work for you by passing an additional request parameter with the API call: units: 'imperial' - will return the temperature in Fahrenheit
See the docs here: https://openweathermap.org/current#data
And here's how I use it in the solution: https://github.com/jsd20191008/weather-api-app-solution/blob/master/js/main-promises.js#L56
|
@kcycle great work! Take a look at the solution if you're interested in one way of approaching the bonus requirement (re: support for zip codes) https://github.com/jsd20191008/weather-api-app-solution/blob/master/js/main-promises.js#L48 |
No description provided.