Conversation
| @@ -1,4 +1,4 @@ | |||
| # Wave 6: Resetting the City Name | |||
| Wave 6: Resetting the City Name | |||
There was a problem hiding this comment.
Looks like this was mistakenly updated
ameerrah9
left a comment
ameerrah9
left a comment
There was a problem hiding this comment.
Great work on this project Ihovanna! 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 🟡
| <div class="container" id="outermost-margin"> | ||
| <div class="weather-box" id="left-box"> | ||
| <p id="current-date">Today's date</p> | ||
| <p id="display-city"> City</p> | ||
| <section id="sky"></section> | ||
| <section id="temperature-container" class="container"> | ||
| <button type="button" id="decrease-temp-button" class="button">❄️</button> | ||
| <p id="display-temperature">Temperature</p> | ||
| <button type="button" id="increase-temp-button" class="button">🔥</button> | ||
| </section> | ||
| <section id="landscape">emojis</section><br> | ||
| <button type="button" id="current-temp-button" class="button">See current weather</button> | ||
| <br> | ||
| <button type="button" id="reset-city-button" class="button">reset city</button> | ||
| </div> | ||
|
|
||
| <div class="weather-box" id="right-box"> | ||
| <select name="weather" id="weather-dropdown" class="dropdown"> | ||
| <option value=" ">Choose your sky!</option> | ||
| <option value="sun">Sunny</option> | ||
| <option value="clouds">Cloudy</option> | ||
| <option value="rain">Rainy</option> | ||
| <option value="snow">Snowy</option> |
| let currentCity = 'San Diego, CA, USA' | ||
| document.getElementById('display-city').textContent = currentCity; | ||
|
|
||
| let temperatureNumber = 70; |
There was a problem hiding this comment.
Consider storing your default state in an object like so:
const state = {
city: 'San Diego, CA, USA',
lat: 47.6038321,
long: -122.3300624,
temp: 70,
};
| let secondCall = findWeather(lat,long); | ||
| secondCall; |
There was a problem hiding this comment.
We don't need an extra variable to hold the function call here
| temperatureResponse = Math.floor(response.data['main']['temp']); | ||
| // weatherResponse = JSON.stringify(response.data['weather'][0]['main']); | ||
| // console.log(`Successful weather response: ${weatherResponse}`) | ||
| temperatureNumber = temperatureResponse |
There was a problem hiding this comment.
I noticed you aren't converting kelvin to farenheit which throws off your changeLandscape logic since you're using farenheit to determine which landscape to display.
| const upButton = document.getElementById('increase-temp-button'); | ||
| upButton.textContent = '🔥'; | ||
| upButton.addEventListener("click", function() { | ||
| temperatureNumber += 1; | ||
| logTempText(); | ||
| changeTextColor(); | ||
| logLandscape(temperatureNumber); | ||
| }); |
There was a problem hiding this comment.
This works, but it would make sense to have a reference to these functions instead of calling them in the event listener
| logTempText(); | ||
| changeTextColor(); | ||
| logLandscape(temperatureNumber); |
There was a problem hiding this comment.
Prefer to register this to be called in response to the DOMContentLoaded event. In the similar live code we did, we did call our onLoaded function directly, since codesandbox got in the way of our code being able to see the DOMContentLoaded event.
| dropdownValue.addEventListener('change', function handleChange () { | ||
| let newSky = changeSky(dropdownValue.options[dropdownValue.selectedIndex].value); | ||
| displaySky.textContent = `${newSky}`; | ||
| display |
There was a problem hiding this comment.
I don't believe this line is needed 😁
| const resetCity = document.getElementById('reset-city-button'); | ||
| resetCity.addEventListener("click", function() { | ||
| currentCity = 'San Diego, CA, USA'; | ||
| document.getElementById('display-city').textContent = currentCity; | ||
| document.getElementById('text-box').value=''; | ||
| }) |
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 resetting the temperature of the displayed city.
There was a problem hiding this comment.
Great work, Ihovanna! 🥳
Thank you for your patience as we catch up on grading. Nice work! The HTML is structured well and the JS is clear and well-factored. However, the requirements for wave 2 and 3 were not met. I've left comments on your PR. This makes the project a yellow. 🟡
Keep it up 💯
2 participants
No description provided.