gloria/scissors #71
gloria/scissors #71
Conversation
There was a problem hiding this comment.
Good work on this project. You've combined javascript, html, and css to create a dynamic website! I've left some inline comments with a few ideas on ways to refactor, make your html more semantic, and move the onclick handling into the javascript file. Please let me know if you have any questions.
| const townnameContainer = document.getElementBy("town-name"); | ||
| }; | ||
|
|
||
| const setTempClass = (tag, className) => { |
There was a problem hiding this comment.
Clever! An alternative to removing all the classes we don't need, and adding the class we do need is using this attribute tag.className = "red-temp"
| const decreaseTempArrow = document.querySelector("#decreaseTempArrow"); //# is id | ||
| decreaseTempArrow.addEventListener('click',decreaseTemp); | ||
| let select = document.querySelector("#sky-card > select"); //select is tag | ||
| select.addEventListener('change',changeSky); |
There was a problem hiding this comment.
Consider moving all the Event Listener logic into a function called registerEventHandlers and add this code document.addEventListener("DOMContentLoaded", registerEventHandlers); to increase readability and changeability.
| <div id="up-arrow" onclick="increaseTemperature()">⬆️</div> | ||
| <div id="decreaseTempArrow">⬇️</div> |
There was a problem hiding this comment.
These arrows are buttons! It is best practice to use semantic HTML and make these button elements.
In addition, rather than putting the onclick event right into the html, we encourage you to use javascript to do this and add the event listener the same way you did for decreaseTemperature
| <div id="up-arrow" onclick="increaseTemperature()">⬆️</div> | |
| <div id="decreaseTempArrow">⬇️</div> | |
| <button id="up-arrow" onclick="increaseTemperature()">⬆️</button> | |
| <button id="decreaseTempArrow">⬇️</button> |
| @@ -0,0 +1,109 @@ | |||
|
|
|||
|
|
|||
| const tempToColor = (temp) => { | |||
There was a problem hiding this comment.
Notice that tempToColor and tempToLandscape have similar logic. Consider how you might combine these two functions to DRY up your code.
| } | ||
| }; | ||
|
|
||
| const skyToLandscape = (sky) => { |
There was a problem hiding this comment.
I like how you encapsulated this logic in a function that returns the value that you need, and then you can invoke the function and use the return value in the event handler.
No description provided.