Sandra Caballero -Spruce #76
Conversation
There was a problem hiding this comment.
Overall, nice job on your site. Your layout looks really clean (I especially like the gradient background). Your ground and color changing works great! Sky handling would be very similar to how the ground is selected, but we would use a 'select' tag and the 'change' event. The city name would be a little different (using the input event instead), but it would also be similar in that we would read the value from the text input, and then update an element in the DOM with that value.
| @@ -4,9 +4,27 @@ | |||
| <meta charset="UTF-8"> | |||
| <meta http-equiv="X-UA-Compatible" content="IE=edge"> | |||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | |||
| <link rel="stylesheet" href="./style.css"/> | |||
There was a problem hiding this comment.
Prefer to put the stylesheets in the styles folder.
| <div id="floor-container">🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷</div> | ||
| <button type="button" id="up-button">Up</button> | ||
| <button type="button" id="down-button">Down</button> | ||
| <span type="number" id="temperature" value="75" class="orange">70</span> |
There was a problem hiding this comment.
The type and value attributes here only apply to an input tag. span doesn't know what to do with them.
|
|
||
| body { | ||
| height: 100vh; | ||
| display: flex; |
| color:antiquewhite; | ||
| } | ||
|
|
||
| .location, .temperature { |
There was a problem hiding this comment.
Nice way to apply a set of style rules to more than one class!
| justify-content: space-around; | ||
| align-items: center; | ||
| } | ||
| .orange{ |
There was a problem hiding this comment.
Class names like these are fine, but in general, try to use class names that convey a sense of the element's role rather than being directly related to its appearance. Here, it's not too big of an issue, since they really don't have much of any other role.
| @@ -0,0 +1,40 @@ | |||
| document.getElementById("up-button").onclick = function(){ | |||
| var tempInt = parseInt(document.getElementById("temperature").innerHTML); | |||
There was a problem hiding this comment.
Consider introducing a state variable outside the functions to hold the temperature, and only writing it to the display element, rather than using the text in the display element as the storage and needing to parse it back.
| @@ -0,0 +1,40 @@ | |||
| document.getElementById("up-button").onclick = function(){ | |||
There was a problem hiding this comment.
Prefer using addEventListener to register event handlers on an element rather than assigning to the on* event attributes.
document.getElementById("up-button").addEventListener("click", function(){
...
});| var tempInt = parseInt(document.getElementById("temperature").innerHTML); | ||
| tempInt--; | ||
| document.getElementById("temperature").innerHTML = tempInt; | ||
| tempColor(tempInt); |
There was a problem hiding this comment.
Notice how similar the body of this function is to the event handler registers for increasing the temperature. Consider making a helper function to hold the parts in common. Just keep the parts that differ in these functions, and make use of the new helper for the things they do that are the same.
| } else { | ||
| color = 'teal'; | ||
| floor = '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲'; | ||
| } |
There was a problem hiding this comment.
Consider the above logic and how similar all the branches are. We look for whether the temperature is within a range, and pick a color and landscape 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 tempRecords = [
{ lowerBound: 80, color: "red", landscape: "🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂"},
{ lowerBound: 70, color: "other color", landscape: "other landscape"},
...
];For the else case, we might set the lowerBound to a very low value, or to null and write a special case to check it. But for everything else, we could simply iterate through the list and find the first record that has a lower bound lower than our temperature, then use it as the source of picking the color and landscape!
No description provided.