-
Notifications
You must be signed in to change notification settings - Fork 78
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
Scissors - Marisa M #53
base: main
Are you sure you want to change the base?
Conversation
…emperature buttons
…tors the Temp buttons.
…eholder text, text styles and landscape text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this creative project. You've created a dynamic website with javascript, html, and css. Nice work! Your code is logical and readable. I've left a few inline comments on ways you might consider refactoring. Please let me know if you have any questions.
<li> <h4 id="locationDisplay"> | ||
Unceded Coast Salish Land</h4> </li> | ||
|
||
<li> <h2 id= "sassy-subtitle"> *Whether or not this report matches the weather you are experiencing is completely coincidental* </h2> </li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the sassy subtitle :)
} else if (currentTemp <= 49) { | ||
color = 'blue' | ||
}; | ||
document.getElementById("temperatureCount").style.color = color; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using classes and than adding this style to the css.
document.getElementById("temperatureCount").style.color = color; | |
document.getElementById("temperatureCount").className = color; |
#styles.css
.blue {
color: blue
}
<div class="left-column"> | ||
|
||
<div class="weather-item" id=temperature> | ||
<h3 id="temperatureCount">Temperature 65℉</h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding an initial color so that when the page is loaded the color matches the 65℉ temperature
document.body.style.background = 'linear-gradient(to bottom, #ffff99 0%, #cc0000 100%)'; | ||
} else if (currentTemp >= 70) { | ||
landscape= 'Good day to sit by the pool'; | ||
document.body.style.background = 'linear-gradient(to bottom, #ffffcc 0%, #ff6600 100%)'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as with the temperature, consider how we could add a className to the element in the javascript, and use this class to select for the element and change the style in our style sheet.
Additionally, note that updateLandscape
has similar code to changeTempColor
. Is there a way to combine these function to DRY up your code?
}; | ||
|
||
|
||
const registerEventHandlers = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work organizing these all together to enhance readability and changeability.
|
||
|
||
<footer> | ||
<p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✊
No description provided.