Phoenix - Alejandra Guevara and Tram Hoang

[tramhn001]

Completed all 6 waves, and testing with proxy server.

[anselrognlie]

Looks good! Please review my comments, and let me know if you have any questions. Nice job!

[anselrognlie]

Since we import this script file at the end of our HTML, these lookups should all work as expected. But we generally don't want to make this assumption, instead, preferring to perform lookups like this in response to the DOMContentLoaded event.

[anselrognlie]

Nit: typo

const landScape = document.getElementById('landscape');

[anselrognlie]

Consider splitting this into two functiosn, one that is able to figure out which data to use to update the screen, and another that actually applies the supplied values to the screen.

[anselrognlie]

Setting the style attribute is the equivalent of hard coding a style in HTML. In general, prefer to assign a class to an element, and use style rules to effect any desired UI formatting changes.

[anselrognlie]

This large chained if statement is repetative. If we stored the relevant data in a way that we could loop through (direct key lookup isn't as helpful, since our temperatures have ranges) we could make our application more data driven, so that just by changing a data structure (that maybe we could load from an API call) there could be different temperature behaviors.

Consider how we could use a structure like the following:

// each row holds the bottom of the temperature range, a class name, and a landscape string
const TEMPERATURE_DATA = [
    [80, 'very-hot-temp', '🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂'],
    [70, 'hot-temp', '🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷'],
    [60, 'warm-temp', '🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃'],
    [50, 'cool-temp', '🌲🌲🍂🌲🍁🍃🌲🌾🌲🌲🍂🍁🌲'],
    [40, 'cold-temp', '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲'],
];

[anselrognlie]

Is there a way we could use data like this to fill in the available sky picks in the UI rather than hard coding them in the HTML?

[anselrognlie]

Try to avoid defining large functions here in the handler for DOMContentLoaded. Or if you are doing so in order to use the handler to provide scope restrictions for your other variables and functions, then do so consistently. (e.g., move all of the other globals here, or move these to be globals).

If everything were moved here, then the variables that lookup the elements could all legitimately by const values. If they were defined globally, but only resolved here, then they would need to be defined as let variables.

[anselrognlie]

Rather than implementing this as a single monolithic function, consider decomposing this into a few smaller functions.

1. a function getLatLonForLocation that takes a name to query and returns {lat, lon} data
2. a function getWeatherForLatLon that takes a {lat, lon} data object and returns weather data

These two functions could be chained together through a third function getWeatherForLocation that takes a name to query and returns weather data by making use of the two smaller functions.

Additional functions could use this overall function to update the UI using the weather data results. While writing one long function can be helpful when trying to get everything working, it's worth goin back, identifying key pieces, and extracting them into helper functions with descriptive names, as this helps our code be more self-documenting.

[anselrognlie]

Nice use of async/await to simplify the logic flow.

[anselrognlie]

Rather than hard-coding the URL, it can be helpful to store the base url in a variable that we also interpolate into the string here. This can make deployment easier (since we'll need to change any dev-routed endpoints to point to their production locations).

const BASE_URL = 'http://127.0.0.1:5000';

then here

                `${BASE_URL}/location?q=${cityName}`

[anselrognlie]

Prefer to pass query params using a data structure if supported by the HTTP library (axios does), as this can prevent unexpected values being injected into the query string. Notice that here, the user's city name input is directly injected into the URL. They could include their own query params in the input (e.g., Seattle&token=1234) which would get added to url as additional params. With our current system, this doesn't lead to a problem, but there could be ways for users to issue problematic requests this way. By making use of any library method for adding params, this will usually encode the params in a way so that they won't be misinterpretted.

axios.get(url, {
  params: {
    q: cityName,
  },
});

[anselrognlie]

Consider making a conversion helper function.