Conversation
buttons still work.
…e on page. Will fix later
…ealtime temp still not working
…oves buttons and removes unused code
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Nice work on weather report, Alaere and Anika! Let me know if you have any questions.
| } | ||
| }); | ||
|
|
||
| const weatherEmojisandColor = () => { |
There was a problem hiding this comment.
Consider renaming this method to use a verb to indicate that this method does something, maybe something like setWeatherEmojisaAndColor
| const state = { | ||
| city: 'Portland, OR', | ||
| temperature: 72, | ||
| latitude: 45.5152, | ||
| longitude: 122.6784, | ||
| }; |
| document.getElementById("sky").addEventListener('change', (event) => { | ||
| const bgValue = event.target.options[event.target.selectedIndex].value; | ||
| if( bgValue == 'sunny' ) { | ||
| document.body.style.backgroundImage = "url(/ada-project-docs/assets/sunshine.png)"; | ||
| } else if( bgValue == "cloudy" ){ | ||
| document.body.style.backgroundImage = "url(/ada-project-docs/assets/cloudy.png)"; | ||
| } else if( bgValue == "rainy" ){ | ||
| document.body.style.backgroundImage = "url(/ada-project-docs/assets/rain.png)"; | ||
| } else if( bgValue == "snowy" ){ | ||
| document.body.style.backgroundImage = "url(/ada-project-docs/assets/snow.png)"; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Could this have been wrapped inside a function and the function been registered on an event inside registerEventHandlers like you do with your other event handler methods?
For example, this could be put in a method called like updateBackground and then in registerEventHandlers you could have:
const background = document.getElementById("sky");
background.addEventListener('change', updateBackground);Doing so would ensure that your format is uniform throughout the project and makes the code a bit more readable
| const weatherEmojisandColor = () => { | ||
| let temp = state.temperature; | ||
| let color; | ||
| if (temp <= 49) { |
There was a problem hiding this comment.
How could you refactor this method so that it's data driven instead of using several if/else if statements (maybe iterate over a data structure) ?
| } | ||
| let tempTag = document.getElementById("tempTag"); | ||
|
|
||
| tempTag.textContent = String(temp); |
There was a problem hiding this comment.
another way to create a string, without using the String constructor is to just use a template string
`${state.temp}`
| }) | ||
| .catch ((error) => { | ||
| console.log("error no temperature found"); | ||
| })}; |
There was a problem hiding this comment.
Nice work getting practice using async/await and promise with then and catch. You might see a large code base with a mix because different people are working on different parts, but usually when the project is small and we can help it we'll want to keep to one style.
Did you prefer one style over the other?
| nowShowingTemp(); | ||
| }; | ||
|
|
||
| const nowShowingTemp = () => { |
There was a problem hiding this comment.
Consider renaming this method to use a verb, maybe something like updateNowShowingTemp
3 participants
No description provided.