olive - sea turtles - weather report

[olive-lavine]

No description provided.

[kelsey-steven-ada]

Great work Olive! I've left some comments & suggestions, take a look when you have time and let me know if there's anything I can clarify =]

[kelsey-steven-ada]

Were you able to see the landscape images when viewing the page locally? Using Chrome, I pulled the code down & I could see the image descriptions, but to view the the images I had to add a . in front of the paths so that it was looking for the images folder in the same folder index.html was in.

[kelsey-steven-ada]

Nice placing the temp change that will happen in every scenario at the top so that code isn't repeated.

[kelsey-steven-ada]

We need to interact with the same items in the various temperature buckets. We could make a helper function that takes in 3 parameters: a color, and image path, and a string for alt text, and that sets those values if they are not falsy.

const updateImgAndColor = (color, imgPath, altText) => {
    if (color) {
        tempDisplay.style.color = color
    }

    ... (and so on for the other 2 parameters)    
}

Then in the if-statements here we could call:

const updateTempDisplay = (x) => {
    ...
    if (state.temp < 60) {
        updateImgAndColor('#2B835E', null, null);
    }
    if (state.temp < 50) {
        updateImgAndColor('teal', '/images/rainy.png', 'black line drawings of cloud with strong diagonal rain falling');
    }
    ...

[kelsey-steven-ada]

We should generally avoid single letter variable names in favor of ones that describe what they hold. What name or names are more descriptive of what x represents?

[kelsey-steven-ada]

I like the idea of pulling element lookups to the top of the page so they're stored in a variable at the time we need them!

It looks like there's a mix styles across the file with these elements being looked up & stored up here and other elements looked up directly in the functions. For consistency, we would want to pick one style to use across the project, with the caveat that if there is an element that is only conditionally aded to the DOM based on user events, that specific item may need to be looked up later inside a function when it is guaranteed to exist.

[kelsey-steven-ada]

We want to remove comments and development & debugging aids before we open PRs to make sure we don't accidentally include them into production/shipping code.

[kelsey-steven-ada]

By setting the style attribute directly on an element, we’re doing the javascript equivalent of adding inline CSS styles, which is not considered best practice. The preference would be to create CSS rules for the styles, likely attached to a class name, then set the element's className attribute to the appropriate value for the style we want to attach.

So in our CSS we'd have rule sets:

...

.red {
    color: red;
}

.orange {
    color: orange;
}

... /* (and so on for the other colors necessary) */

And here we'd do something like:

...
if (state.temp >= 80) {
    currentTemp.className = 'red';
...

[kelsey-steven-ada]

Great use of async/await to let you separate the network call functions!

[kelsey-steven-ada]

In production code we'd likely want to include comments about what the hex colors are, since many folks cannot sight-read hex values.

[kelsey-steven-ada]

Really nice use of helper functions to divide up the actions we want to take into smaller portions that are easier to read and test.