Conversation
ameerrah9
left a comment
ameerrah9
left a comment
There was a problem hiding this comment.
For commits, make sure to commit more often and make your commits more descriptive! Instead of taking about what wave was completed, include details about what functionality was added.
| <section class="sky_section"> | ||
| <h2>Sky selection</h2> | ||
|
|
||
| <select id="selectSky"> | ||
| <option >Sunny</option> | ||
| <option >Cloudy</option> | ||
| <option >Rainy</option> | ||
| <option >Snowy</option> | ||
| </select> | ||
|
|
||
|
|
||
| </section> |
There was a problem hiding this comment.
Be careful with indentation here
|
|
||
|
|
||
| <div > | ||
| <h2 id="city-display">✨Atlanta✨</h2> |
There was a problem hiding this comment.
Avoid using hard-coded values in our markup
| <button id="decrease_temperature">↓</button> | ||
| <button type="button" id="realtime-temp">Get Realtime Temperature</button> | ||
| </div> | ||
| <span id="temperatureDisplay" class="orange">72</span> |
There was a problem hiding this comment.
Rather than hard-coding a value for UI elements (which just happens to match the initial value backing the control), I like to leave the initial markup blank, and update the UI on first load from the initial backing values in the JS code. So like here, leave off the 72, then when the document loads, update the UI in the JS to show the initial temperature value. This would protect us from changing the default starting JS value, but having it be out of sync with what the UI is showing.
I would tend to do this for any of the configurable elements (temperature, ground, sky, city).
| .weather-header { | ||
| display: flex; | ||
| flex-direction: column; | ||
| grid-column: auto / span 2; | ||
| color: white; | ||
|
|
||
| } | ||
| body { | ||
| background-color: rgb(136, 195, 241); | ||
| } | ||
|
|
||
| .orange { | ||
| color: rgb(255, 145, 49); | ||
| } | ||
| #temperatureDisplay{ | ||
| display: flex; | ||
| font-size: xx-large; |
There was a problem hiding this comment.
Nice work with the styling, though I think you could have pushed yourself to add some CSS grid styling
| @@ -0,0 +1,148 @@ | |||
| const getTempFromCoordinates = () => { | |||
| const cityInput = document.getElementById('city-display').textContent; | |||
| console.log(cityInput); | |||
There was a problem hiding this comment.
Remember to remove debugging log statements
| axios | ||
| .get('http://127.0.0.1:5000/location', { | ||
| params: { | ||
| q: cityInput, | ||
| }, | ||
| }) | ||
| .then((response) => { | ||
| console.log('success', response); | ||
| const return_obj = { | ||
| lat: response.data[0].lat, | ||
| lon: response.data[0].lon, | ||
| }; | ||
| axios | ||
| .get('http://127.0.0.1:5000/weather', { | ||
| params: { | ||
| lat: return_obj['lat'], | ||
| lon: return_obj['lon'], | ||
| }, | ||
| }) | ||
| .then((weatherResponse) => { | ||
| console.log(weatherResponse); | ||
| const cityTemp = weatherResponse.data['main']['temp']; | ||
| let tempDisplay = document.getElementById('temperatureDisplay'); | ||
| console.log(cityTemp); | ||
| const tempFahrenheit = | ||
| (parseInt(cityTemp) - parseInt(273.15)) * 1.8 + 32; | ||
| console.log(tempFahrenheit); | ||
| tempDisplay.textContent = tempFahrenheit; | ||
|
|
||
| changeTempColor(tempDisplay); | ||
| const garden = gardenChange(tempDisplay); | ||
|
|
||
| const gardenContainer = document.getElementById('garden_section'); | ||
| gardenContainer.textContent = garden; | ||
| }); | ||
| }) |
There was a problem hiding this comment.
Consider breaking this function up a bit. It's taking on a lot responsibilty. Whenever we see a function that is more than 10 lines long we should consider seprating concerns. This is completely preferential but just a note. More on this here:
https://dmitripavlutin.com/the-art-of-writing-small-and-plain-functions/
There was a problem hiding this comment.
There's an issue with wave 04 (calling the apis), the requirements specify that your code should get the weather of the currently displayed city name & when the element is clicked updates and displays the realtime temperature of the currently displayed city name. Currently your code is not providing the updated temperature.
| const inputCity = document.getElementById('city-name-input').value; | ||
| const displayCityContainer = document.getElementById('city-display'); | ||
|
|
||
| displayCityContainer.textContent = '✨ ' + inputCity + ' ✨'; |
There was a problem hiding this comment.
You could also use concatentation here like so:
displayCityContainer.textContent = `✨ ${inputCity} ✨`;
| const registerEventHandlers = () => { | ||
| const increaseTempButton = document.getElementById('increase_temperature'); | ||
| increaseTempButton.addEventListener('click', increaseTemp); | ||
|
|
||
| const decreaseTempButton = document.getElementById('decrease_temperature'); | ||
| decreaseTempButton.addEventListener('click', decreaseTemp); | ||
|
|
||
| const cityInputForm = document.getElementById('city-name-input'); | ||
| cityInputForm.addEventListener('input', displayCityName); | ||
|
|
||
| updateSky(); | ||
| const skySelect = document.getElementById('selectSky'); | ||
| skySelect.addEventListener('change', updateSky); | ||
|
|
||
| const resetButton = document.getElementById('reset-city-name-2'); | ||
| resetButton.addEventListener('click', resetCityName); | ||
|
|
||
| const realtimeTempButton = document.getElementById('realtime-temp'); | ||
| realtimeTempButton.addEventListener('click', getTempFromCoordinates); | ||
| }; |
ameerrah9
left a comment
ameerrah9
left a comment
There was a problem hiding this comment.
Great work on this project Maria! The code you wrote for wave 4 is really close, I've put in a suggested solution, but I didn't have time to test it so it may need some adjustments. The code in this project is really clean and easy to read. This project is yellow 🟡
2 participants
No description provided.