-
Notifications
You must be signed in to change notification settings - Fork 88
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
Sarah Williams + Morgan Adkisson - Whales #65
base: main
Are you sure you want to change the base?
Conversation
…ting, helper function added for text color and garden landscape change depending on temp ranges
…itions object created to hold values
…updated with the daily temps when the real time button is pressed
…nd .catch to axios calls
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.
Nice wireframe illustration!
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 job!
I left a few small comments but those are all pretty minor.
Well done!
@@ -0,0 +1,144 @@ | |||
const GARDENSKIES = { |
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 really like this approach! Very clean and extensible!
} else if (temp < 50) { | ||
currentCondition = CONDITIONS.cold; | ||
} | ||
el.style.color = currentCondition.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.
Using class names for this would have put all the styling in one location, though admittedly having your settings at the top of the file makes that less necessary.
const CONDITIONS = { | ||
hot: { landscape: '🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂', text: 'red' }, | ||
warm: { landscape: '🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷', text: 'orange' }, | ||
moderate: { landscape: '🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃', text: 'yellow' }, | ||
chilly: { landscape: '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲', text: 'green' }, | ||
cold: { landscape: '❄️🌲⛄️🌲❄️🏂⛄️🌲❄️⛷🌲❄️🌲', text: 'teal' }, | ||
}; |
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.
Style: I it would have been clearer if you called the key color
instead text
:
const CONDITIONS = { | |
hot: { landscape: '🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂', text: 'red' }, | |
warm: { landscape: '🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷', text: 'orange' }, | |
moderate: { landscape: '🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃', text: 'yellow' }, | |
chilly: { landscape: '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲', text: 'green' }, | |
cold: { landscape: '❄️🌲⛄️🌲❄️🏂⛄️🌲❄️⛷🌲❄️🌲', text: 'teal' }, | |
}; | |
const CONDITIONS = { | |
hot: { landscape: '🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂', color: 'red' }, | |
warm: { landscape: '🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷', color: 'orange' }, | |
moderate: { landscape: '🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃', color: 'yellow' }, | |
chilly: { landscape: '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲', color: 'green' }, | |
cold: { landscape: '❄️🌲⛄️🌲❄️🏂⛄️🌲❄️⛷🌲❄️🌲', color: 'teal' }, | |
}; |
border-radius: 25px; | ||
} | ||
|
||
#weatherFrame { |
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.
It looks like you leaned pretty hard on ids here, which does make it easier to style things but slightly hampers re-usability of styles.
No description provided.