C24 Possum/Crow Esmeralda & Riley#6
Conversation
… some room to refactor in the changeSky function
… text to change the sky color and text
mikellewade
left a comment
mikellewade
left a comment
There was a problem hiding this comment.
@rileydrellishak & @esmerarre, nice work on your weather report project! Sorry for the delay! Even instructors have a backlog sometimes!
| @@ -0,0 +1,179 @@ | |||
| const proxyServerURL = 'https://ada-weather-report-proxy-server.onrender.com'; | |||
There was a problem hiding this comment.
Since this variable is a true constant, you could follow the convention of the naming the variable in upper/snakecase.
const BASE_URL = 'https://ada-weather-report-proxy-server.onrender.com';| const proxyServerURL = 'https://ada-weather-report-proxy-server.onrender.com'; | ||
|
|
||
| const findLatAndLon = (cityName) => { | ||
| let lat, lon; |
There was a problem hiding this comment.
Declaring these variable here makes me think that they will be accessed somewhere in addition to the assignment located in your .then chain. Being they are only being declared here, I would suggest you move the declaration/assignment into the .then entirely. This would also be more secure since it would be locally scoped to the callback of the .then.
| let lat, lon; |
| lat = response.data[0].lat; | ||
| lon = response.data[0].lon; |
There was a problem hiding this comment.
| lat = response.data[0].lat; | |
| lon = response.data[0].lon; | |
| const lat = response.data[0].lat; | |
| const lon = response.data[0].lon; |
| .then( (response) => { | ||
| lat = response.data[0].lat; | ||
| lon = response.data[0].lon; | ||
| return {lat, lon}; |
There was a problem hiding this comment.
| return {lat, lon}; | |
| return { lat, lon }; |
| }; | ||
|
|
||
| const getWeatherFromLatAndLon = (latitude, longitude) => { | ||
| let temp; |
There was a problem hiding this comment.
Same feedback as the let lat, lon; declaration/assignment.
| let currentText = document.querySelector('#cityNameInput').value; | ||
| let cityHeader = document.querySelector('#headerCityName'); |
There was a problem hiding this comment.
| let currentText = document.querySelector('#cityNameInput').value; | |
| let cityHeader = document.querySelector('#headerCityName'); | |
| const currentText = document.querySelector('#cityNameInput').value; | |
| const cityHeader = document.querySelector('#headerCityName'); |
| let currentText = document.querySelector('#cityNameInput'); | ||
| let cityHeader = document.querySelector('#headerCityName'); |
There was a problem hiding this comment.
| let currentText = document.querySelector('#cityNameInput'); | |
| let cityHeader = document.querySelector('#headerCityName'); | |
| const currentText = document.querySelector('#cityNameInput'); | |
| const cityHeader = document.querySelector('#headerCityName'); |
| const registerEventHandlers = () => { | ||
| const increaseTempControl = document.querySelector('#increaseTempControl'); | ||
| increaseTempControl.addEventListener('click', increaseTemp); | ||
|
|
||
| const decreaseTempControl = document.querySelector('#decreaseTempControl'); | ||
| decreaseTempControl.addEventListener('click', decreaseTemp); | ||
|
|
||
| const changeSkyControl = document.querySelector('#skySelect'); | ||
| changeSkyControl.addEventListener('change', changeSky); | ||
|
|
||
| const cityText = document.querySelector('#cityNameInput'); | ||
| cityText.addEventListener('keyup', updateCityName); | ||
|
|
||
| const resetCity = document.querySelector('#cityNameReset'); | ||
| resetCity.addEventListener('click', resetCityNameButton); | ||
|
|
||
| const getCurrentTemp = document.querySelector('#currentTempButton'); | ||
| getCurrentTemp.addEventListener('click', getWeatherFromCityName); | ||
| }; |
| const loadControls = () => { | ||
| state.tempValue = document.getElementById('tempValue'); | ||
| state.tempValue.textContent = state.tempNum; | ||
| state.tempValue.classList.add('orange'); | ||
|
|
||
| state.gardenLandscape = document.getElementById('landscape'); | ||
| state.gardenLandscape.textContent = state.gardenLandscapeText; | ||
|
|
||
| state.gardenSkyline = document.getElementById('sky'); | ||
| state.gardenSkyline.textContent = state.gardenSkyText; | ||
|
|
||
| state.gardenContainer = document.getElementById('gardenContent'); | ||
| state.gardenContainer.classList.add('sunny'); | ||
|
|
||
| state.headerCityName = document.getElementById('headerCityName'); | ||
| state.headerCityName.textContent = state.cityName; | ||
| }; |
| const onLoaded = () => { | ||
| loadControls(); | ||
| registerEventHandlers(); | ||
| }; | ||
|
|
||
| onLoaded(); No newline at end of file |
There was a problem hiding this comment.
I would suggest just moving the logic of adding event handlers to their respective HTML elements into something like document.addEventListener("DOMContentLoaded", RegisterEventHandlers);. That way we register the event handler functions to run when the DOM content has fully loaded. This ensures all HTML elements and resources are available before attempting to attach event handlers or manipulate the DOM.
3 participants
No description provided.