Conversation
mmcknett-ada
left a comment
mmcknett-ada
left a comment
There was a problem hiding this comment.
Hey Edi & Janice, great job! 🟢
You were really close to having everything working perfectly, there were just a few minor issues:
- Needed to change
updateSky-->resetSky - Needed to pass the correct querystring params to the /weather endpoint
The only things to watch out for besides that are code indent/spacing style, naming, and use of semantic HTML.
I really like how you styled your page with CSS. Well done!
| axios.get('http://127.0.0.1:5500/weather', | ||
| { | ||
| params: { | ||
| q: state.city, |
There was a problem hiding this comment.
This was expecting you to pass lat and lon parameters. This causes you to get a response that says {"message":"must provide lat and lon parameters"} and makes your .catch run because weather ends up being undefined on line 23.
There was a problem hiding this comment.
| q: state.city, | |
| lat: state.lat, | |
| lon: state.long |
| color: yellow | ||
| } | ||
| .yellow-green { | ||
| color:yellowgreen |
There was a problem hiding this comment.
[nit] try to consistently space
| color:yellowgreen | |
| color: yellowgreen |
|
|
||
| <section id="city"> | ||
| <h2>City Name</h2> | ||
| <input type="text" id="textInput" value="Seattle"> |
There was a problem hiding this comment.
[nit] cityInput might be better for id since its more descriptive.
| </div> | ||
| </section> | ||
|
|
||
| <section id="sky" onclick="resetSky()"> |
There was a problem hiding this comment.
It's a little confusing if some elements have an onclick defined and some elements have their click handlers registered for in the .js file (in registerEventHandlers). It's probably better to pick one or the other.
| <body> | ||
| <script src="./src/index.js"></script> | ||
|
|
||
| <section> |
There was a problem hiding this comment.
[nit] Try to indent consistently. This whole <section> should be tabbed one to the right.
| font-size: 2em; | ||
| } | ||
|
|
||
| #text_weather { |
There was a problem hiding this comment.
[nit] I'm used to seeing - to separate words, e.g. text-weather
| <span id="for_city"> <h3>For the city of</h3> | ||
| <div id="output">Seattle</div> | ||
| </span> |
There was a problem hiding this comment.
It's a little strange to have two block-level elements inside a line-level element. Probably better to use div instead of span here and on text_weather. Also, h3 seems like it's not being used semantically, since there's no intervening h2. You might prefer to just define a class with the styling you want and apply that in a span or div.
| <header> | ||
| <span id="text_weather"> | ||
| <h1 > Weather Report</h1> | ||
| <span id="for_city"> <h3>For the city of</h3> | ||
| <div id="output">Seattle</div> | ||
| </span> | ||
|
|
||
| </span> | ||
| </header> |
There was a problem hiding this comment.
Indentation & spacing nits
| <header> | |
| <span id="text_weather"> | |
| <h1 > Weather Report</h1> | |
| <span id="for_city"> <h3>For the city of</h3> | |
| <div id="output">Seattle</div> | |
| </span> | |
| </span> | |
| </header> | |
| <header> | |
| <span id="text_weather"> | |
| <h1> Weather Report</h1> | |
| <span id="for_city"> | |
| <h3>For the city of</h3> | |
| <div id="output">Seattle</div> | |
| </span> | |
| </span> | |
| </header> |
| }; | ||
|
|
||
| const findLatitudeAndLongitude = () => { | ||
| let lat, long; |
There was a problem hiding this comment.
You can remove these since you're using state.lat and state.long
|
|
||
| resetSky(); | ||
| const skySelect = document.getElementById('skySelect'); | ||
| skySelect.addEventListener('change', updateSky); |
There was a problem hiding this comment.
There's no updateSky, and you have resetSky hooked up to the click handler of the sky select box. That's why your sky doesn't update after you select the type of sky, but it does update after you click the selector for a second time.
There was a problem hiding this comment.
Just hooking up resetSky seems to fix this.
| skySelect.addEventListener('change', updateSky); | |
| skySelect.addEventListener('change', resetSky); |
3 participants
No description provided.