C24 Possum Alice and Makiko

[trimakichan]

No description provided.

[anselrognlie]

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

[anselrognlie]

👍 Nice use of a variable to hold the remote base url for the API calls. While there's no "standard" way to manage this value in plain JS to set it appropriately for either local dev or remote deployed values, this is a good first step that at least means that we only need to worry about managing a single value.

[anselrognlie]

👍 Excellent job isolating the API calls (and the combination of the two) in this file, which has no special knowledge of the rest of the application. These focus on isolating the details of the API calls from the rest of the application, extracting only the data the application needs.

[anselrognlie]

Consider putting these conversion calculations in their own helper functions.

[anselrognlie]

👍 Great use of then chaining to combine these functions, and returning the resulting promise ensures that additional (application specific) operations can be chained where this function is used.

[anselrognlie]

Using catch within the helpers can be fine for performing some output logging, but note that doing so "clears" the error, which would allow any chained thens to continue to run. For example, if there was an error fetching the coordinates, then in your later findWeatherForCity, the chained call to findCurrentWeather would still run, since the catch "handled" the error. However, the value passed in to the chain would be undefined, which would cause problems.

So in order to prevent the rest of the chain from executing, rethrow the error (just throw error;) so that the error continues to propagate and skip following thens. In your main logic chain, you would add one final catch at the very end to prevent the error from falling off the end entirely (which the browser will complain about).

Really, if you're not going to do specific error handling in the helper itself (such as returning a default result, which could be passed along the chain, or converting the axios error to something more specific to the helper operation), it might be best to leave the catches off and just do the logging at the end of the promise chain.

[anselrognlie]

This is a great idea to store the temperature configuration as data. The specified behavior of groups of 10 degrees makes your approach below towards binning the temperature work well.

Note that even when things don't line up so nicely, we can still take a similar approach, by making the boundary values a part of the records, and iterating through them as a list rather than trying to make use of an object property.

const TEMP_STYLES = [
  {
    lower: 80,
    class: 'red',
    landscape: '🌵🐍🦂🌵🐍🦂🌵🐍🦂🌵🐍🦂🌵🐍🦂🌵🐍🦂'
  },
  {
    lower: 70,
    class: 'orange',
    landscape: '🌼🌻🌱🌼🌻🌱🌼🌻🌱🌼🌻🌱🌼🌻🌱🌼🌻🌱'
  },
  ...  // etc
];

[anselrognlie]

👍 Great job using your temperature dict to make your temperature behavior data driven. As mentioned above, if there isn't a good way to bucket the data, we could still maker use of a similar approach (assuming the previous alternative structure):

  let style = TEMP_STYLES.at(-1);  // default to lowest record
  for (const entry of TEMP_STYLES) {
    if (temp >= entry.lower) {
      style = entry;
      break;
    }
  }

  // style will now have the appropriate entry

[anselrognlie]

👍 Nice function to handle the temperature change.

Rather than passing an abstract "action", we would pass a apecific delta.

const changeTemp = (delta) => {
    state.temp += delta;

We could then call it as changeTemp(1) for the up case, and changeTemp(-1) for the down case.

[anselrognlie]

For the city name here, and the sky value below, rather than updating the UI directly, consider assigning to a state value similar to the temperature, and then having separate logic to reflect the changes in the UI.

In fact, this function could be that separate logic if the value were passed in from the state rather than directly from the triggering event, or other UI element.

Ideally, the "output" parts of the UI should be able to driven from a state representation that isn't dependent on the other parts of the UI. This allows us to set the output arbitrarily based only on in-memory state that's easy to control.

[anselrognlie]

👍 Using your API helpers, this app-specific logic is really clean!