-
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
c17 Sharks - Xiomara Rodriguez #71
base: main
Are you sure you want to change the base?
Conversation
…atement outside a module
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 job, Xiomara! A couple things to consider:
- Remember to remove any commented out code you aren't using for a final submission
- How can we dry up a few of the functions that are a long list of if statements and re-assigning state variables?
Something we could do is fatten up state with all of these values, like the emojis and font colors in aterTextColor
that correlate to the sky or the temperature. For example:
const state = {
stats: [
{ upperBound: 49, color: "teal", ground: "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲"},
{ upperBound: 60, color: "yellow", ground: "🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃"}
]
}
Now we can iterate through our hard coded values that correlate to the right temp or sky by referencing a key just once. That's really all I saw that could be dried up! Nicely done!
dist/index.html
Outdated
<!-- <section id="sky"> | ||
<canvas id="icon" width="128" height="128"></canvas> | ||
</section> --> |
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.
<!-- <section id="sky"> | |
<canvas id="icon" width="128" height="128"></canvas> | |
</section> --> |
dist/index.html
Outdated
<canvas id="icon" width="128" height="128"></canvas> | ||
</section> --> | ||
</div> | ||
<!-- <div id="skyChoice"></div> --> |
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.
<!-- <div id="skyChoice"></div> --> |
dist/index.html
Outdated
<!-- city text display --> | ||
<!-- <section id="citySection"> | ||
<span id="cityDisplay">New York City</span> | ||
<button id="currentTempButton">Get Realtime Temperature</button> | ||
</section> --> |
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.
<!-- city text display --> | |
<!-- <section id="citySection"> | |
<span id="cityDisplay">New York City</span> | |
<button id="currentTempButton">Get Realtime Temperature</button> | |
</section> --> |
dist/index.html
Outdated
</svg> | ||
<!-- <script src="node_modules/axios/dist/axios.min.js"></script> --> | ||
<script src="/src.a2b27638.js"></script> | ||
<!-- <script src="skycons.js"></script> --> |
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.
<!-- <script src="skycons.js"></script> --> |
// change the sky | ||
// const setIcon = (icons, iconID) => { | ||
// const skycons = new Skycons({ color: '#2b2d42' }); | ||
// const currentIcon = icons.replace(/-/g, '_').toUpperCase(); | ||
// skycons.play(); | ||
// return skycons.set(iconID, skycons[currentIcon]); | ||
// }; |
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.
don't forget to get rid of code you aren't using:
// change the sky | |
// const setIcon = (icons, iconID) => { | |
// const skycons = new Skycons({ color: '#2b2d42' }); | |
// const currentIcon = icons.replace(/-/g, '_').toUpperCase(); | |
// skycons.play(); | |
// return skycons.set(iconID, skycons[currentIcon]); | |
// }; |
// let skyContent = document.getElementById('body').style.background | ||
// const skyContent = document.getElementById('skyChoice'); | ||
// let skyScape = ''; |
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.
// let skyContent = document.getElementById('body').style.background | |
// const skyContent = document.getElementById('skyChoice'); | |
// let skyScape = ''; |
src/index.js
Outdated
}; | ||
window.onload = changeSky; | ||
|
||
document.getElementById('skies').addEventListener('change', changeSky); |
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.
hmm should this be inside registerEventHandler
? This will make sure that the DOM has loaded completely before any js is executed. Sometimes a js file can load faster than the HTML itself and cause errors.
decrementButton.addEventListener('click', decrementTemp); | ||
searchCity.addEventListener('input', updateCity); | ||
currentTempButton.addEventListener('click', findLatitudeAndLongitude); | ||
// selectSkies.addEventListener('change', changeSky); |
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.
// selectSkies.addEventListener('change', changeSky); |
src/index.js
Outdated
location.reload(); | ||
}; | ||
|
||
resetButton.addEventListener('click', refreshPage); |
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.
same as above. This might be better placed inside registerEventHandler
src/index.js
Outdated
// const setIcons = () => { | ||
// icons.set('clear-day', Skycons.CLEAR_DAY); | ||
// icons.set('rain', Skycons.RAIN); | ||
// icons.set('snow', Skycons.SNOW); | ||
// icons.set('wind', Skycons.WIND); | ||
|
||
// icons.play(); | ||
// }; |
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.
// const setIcons = () => { | |
// icons.set('clear-day', Skycons.CLEAR_DAY); | |
// icons.set('rain', Skycons.RAIN); | |
// icons.set('snow', Skycons.SNOW); | |
// icons.set('wind', Skycons.WIND); | |
// icons.play(); | |
// }; |
still making some changes, but it's pretty much done!