Stephanie/Crows and Olga/Possums Weather App

[Olga-allgood]

No description provided.

[anselrognlie]

Looks good overall, but please review my comments, and let me know if you have any questions.

[anselrognlie]

Since this script is set to run defered, it should be OK to call onLoaded directly like this and have reasonable certainty that the page elements will have been loaded. Still, consider registering this to run with the DOMContentLoaded event.

[anselrognlie]

👍 I like to load any of the elements I'll be using just once, and then access them later on. Doing so avoids possible typos in the string values used in the lookup value.

[anselrognlie]

👀 This function updates the sky display, but it doesn't get called until the sky picker changes, meaning no sky is shown when the page loads.

[anselrognlie]

The landscape is being calculated from the temperature further down. currentLandscape doesn't appear to be used and can be removed. Looking it up as needed (as you did) is the preferred approach.

[anselrognlie]

The currentWeatherState is also calculated from the temperature. Rather than storing it as a separate value, it could be calculated from the current temperature as needed. If we can calculate one value from another (think back to calculating is_complete from completed_at in Task List API), we should try to avoid storing the dependent data.

[anselrognlie]

Rather than needing to track the previous class in order to remove it, we can either

1. Attempt to remove all possible classes (fine for a small number of classes)

  for (const colorClass of Object.values(COLOR_STATES))
    state.temperatureDisplay.classList.remove(colorClass);
  }

or 2. Since the page structure is set up so that the temperture element should only ever have a single class applied, use the className property instead to replace the class with the new one. Sometimes the element structure needs to be adjusted to accomodate this if there may need to be other structural classes applied, but here, that's not a concern.

  state.temperatureDisplay.className = COLOR_STATES[state.currentWeatherState];

Ideally, as noted below, rather than storing the currentWeatherState, we could just compute it from the temperature.

[anselrognlie]

As with the color, we could give these buckets more meaningful names.

[anselrognlie]

options allow us to set a value so that the JS value we read from the control can be set independent of the values being displayed. This can allow us to pick values that better match logic we're running (such as matching the names of the CSS classes we want to assign).

We could likewise just pick some arbitrary set of ids, and establish the mapping to the CSS class similar to how we can pick a string of emoji.

            <option value="sunny">Sunny</option>
            <option value="cloudy">Cloudy</option>
            <option value="rainy">Rainy</option>
            <option value="snowy">Snowy</option>

[anselrognlie]

Rather than reading the current sky value directly from the sky picker, consider having an event handle that stores the current value into the state, and then separate the logic that updates the UI to read the current sky from the state.

[anselrognlie]

Similar to my recommendation for the sky, rather than updating the UI directly from the control value, have one function that responds to the change events and updates state, and another that updates the UI based on the current state.