Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kassidy Buslach Sharks #81

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Kassidy Buslach Sharks #81

wants to merge 6 commits into from

Conversation

buslakj
Copy link

@buslakj buslakj commented Jun 15, 2022

No description provided.

Copy link

@yangashley yangashley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! Your code was easy to read and I was able to do all of the required tasks in your weather app! Please let me know if you have any questions.

🟢 for weather-report!

Comment on lines +39 to +45
<footer class="seasons">
<h2>Landscape:
<span id="conditions"> 🌸☀️🌸☀️🌸☀️🌸☀️🌸☀️</span>
</h2>


</footer>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this part of the page is at the bottom of the screen when I view it in a browser, it might be more fitting to use a section tag here instead of a footer tag.

A footer typically contains information about the author of the section, copyright data or links to related documents:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/footer

</head>
<header>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Your HTML is semantic and clearly written. Nice job!

Comment on lines +18 to +22
tempChangeColor(state.fahrenheit);
weatherGarden();
const decrease = document.getElementById('f');
decrease.textContent = `Degrees Fahrenheit:${state.fahrenheit}`;
return decrease.textContent;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice lines 18-22 are almost identical to lines 9-13 above. You can put these 5 lines into a helper method like updateTempAndDisplay() and then call that helper method in the tempUp and tempDown methods.

return decrease.textContent;
};

const cityInput = (event) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method name getCityWeather on line 30 is very descriptive because it tells me exactly what the method will do. The name cityInput is vague here. If you refactor this method from cityInput to updateCityInputAndGetRealWeather then you increase readability overall because someone could read the method name and know what the method will do.

Comment on lines +34 to +47
let element = document.getElementById('temperatureContainer');
if (state.fahrenheit >= 80) {
element.style.backgroundColor = 'red';
} else if (80 > state.fahrenheit && state.fahrenheit >= 70) {
element.style.backgroundColor = 'orange';
} else if (70 > state.fahrenheit && state.fahrenheit >= 60) {
element.style.backgroundColor = 'yellow';
} else if (60 > state.fahrenheit && state.fahrenheit >= 50) {
element.style.backgroundColor = 'green';
} else if (50 > state.fahrenheit && state.fahrenheit >= 33) {
element.style.backgroundColor = 'teal';
} else if (state.fahrenheit <= 32) {
element.style.backgroundColor = 'blue';
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out the logic in tempChangeColor(), see how similar the branches are of the conditional statement. We look for whether the temperature is within a range, and pick a color accordingly. What if we had a list of records that we could iterate through to find the values. We could set up something like

const TEMP_COLORS = [ 
  { upperBound: 49, color: "teal"}, 
  { upperBound: 59, color: "green"}, 
  { upperBound: 69, color: "yellow"}, 
  { upperBound: 79, color: "orange"}, 
  { upperBound: 80, color: "red"}
];

Then your method would iterate through TEMP_COLORS and find the first record that has an upper bound higher than our temperature, then use it as the source of picking the color which would help make the function a bit more concise.

This could accommodate a scenario where you might have even more colors in the future, which would prevent your conditional statement from being a really long block of if/elif/elif and so on


const resetCity = (event) => {
const resetInput = document.getElementById('input');
resetInput.value = 'Edmonds';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider creating a constant like DEFAULT_CITY for 'Edmonds' instead of using a string literal here. It's good practice to use constants when the literal value won't change and also if someone reads

cityInput.value = DEFAULT_CITY;

the intent is a little more clear.

@@ -0,0 +1,26 @@
header {text-align: center;}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can change how this is formatted property and value on its own line (like what you did on lines 3-5) to be in line with the rest of the formatting in this file

Comment on lines +10 to +11


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete unnecessary whitespace here before the closing bracket to improve code readability

Comment on lines +5 to +6
}
section.info {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a white space between a closing bracket and the start of the next rule

Comment on lines +17 to +21





Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete whitespace

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants