Phoenix-AnhTran , Sphinx-Tatiana-Snook #7
Conversation
…r style and landscape in weather garden
…y if there is no city add in
…e, change value to empty string whenever click the Reset button for City
…location in order to get the weather, and try to convert to F from Kelvin
…the states of sky
…y server from Render
…ate the gerrealtimetemperraturebutton to return the temperature in city with lat and lon provided
…and garden landscape
| @@ -1,15 +1,59 @@ | |||
| <!DOCTYPE html> | |||
| <html lang="en"> | |||
|
|
|||
There was a problem hiding this comment.
The spacing is being used for readability and separating different parts of the page, this is fine for me. Just be mindful that some developers might prefer for there to be no empty spaces.
| let temperature = 48; | ||
|
|
||
| const tempValue = document.getElementById('tempValue'); |
There was a problem hiding this comment.
Nice work on appropriately using the let and const keywords!
| const increaseTempControl = document.getElementById('increaseTempControl'); | ||
| const decreaseTempControl = document.getElementById('decreaseTempControl'); | ||
| const landScape = document.getElementById('landscape'); | ||
| const cityNameInput = document.getElementById('cityNameInput'); | ||
| const cityNameReset = document.getElementById('cityNameReset'); | ||
| const headerCityName = document.getElementById('headerCityName'); | ||
| const getRealTimeTempButton = document.getElementById('getRealTimeTempButton'); |
There was a problem hiding this comment.
Looks like we are doing similar logic in a few places (finding an element by id and then, later in our code, adding an event handler to it for a specific event). Do you think that this could call for a helper function to make our code a bit more DRY?
| const skyOptions = { | ||
| 'sunny': { | ||
| 'text': "☁️ ☁️ ☁️ ☀️ ☁️ ☁️", | ||
| 'backgroundColor': "#DFFFFF", | ||
| }, | ||
| 'cloudy': { | ||
| 'text': "☁️☁️ ☁️ ☁️☁️ ☁️ 🌤 ☁️ ☁️☁️", | ||
| 'backgroundColor': "#D3D3D3", | ||
| }, | ||
| 'rainy': { | ||
| 'text': "🌧🌈⛈🌧🌧💧⛈🌧🌦🌧💧🌧🌧", | ||
| 'backgroundColor': "#B0C3DF", | ||
| }, | ||
| 'snowy': { | ||
| 'text': "🌨❄️🌨🌨❄️❄️🌨❄️🌨❄️❄️🌨🌨", | ||
| 'backgroundColor': "#ADD9E6", | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Nice way of storing these! You could even put them in another file and just import them to better declutter your code!
| const skySelect = document.getElementById("skySelect"); | ||
| const skyDisplay = document.getElementById("skyDisplay") | ||
| const gardenContent = document.getElementById("gardenContent") | ||
|
|
||
| skySelect.addEventListener("change", (event) => { | ||
| const skySelected = event.target.value; | ||
| console.log("Selected sky:", skySelected); | ||
| updateSky(skySelected); | ||
| }); |
There was a problem hiding this comment.
A few things here:
- The event handler I would put in its own function that why it would be easier to read as well as maintain.
- I would 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. - Don't forget to remove console logs like this, you could accidentally leak private data. Also just helps to keep the code clutter free.
| const city = headerCityName.textContent; | ||
|
|
||
| getCityLocation (city) | ||
| .then(({ lat, lon }) => { | ||
| return getCityTemperature(lat,lon); | ||
| }) | ||
| .catch((error) => { | ||
| console.log('Error data:', error); | ||
| tempValue.textContent = 'Unable to get temperature'; | ||
| }) |
There was a problem hiding this comment.
I would also suggest moving this into its own function(s), things could get tricky to debug (especially working with chained promises inside of callback functions.
|
|
||
| getCityLocation (city) | ||
| .then(({ lat, lon }) => { | ||
| return getCityTemperature(lat,lon); |
There was a problem hiding this comment.
I don't think you actually need to return this since you aren't chaining it to anything else.
| const getCityLocation = (city) => { | ||
| return axios | ||
| .get('https://ada-weather-report-proxy-server.onrender.com/location', {params:{'q': city}}) | ||
| .then((locationResponse) => { | ||
| console.log(locationResponse); | ||
| const lat = locationResponse.data[0].lat; | ||
| const lon = locationResponse.data[0].lon; | ||
| console.log({ lat, lon }); | ||
| return { lat, lon }; | ||
| }) | ||
| .catch((error) => { | ||
| console.log('Error data:', error); | ||
| throw error | ||
| }); | ||
| }; |
| tempValue.textContent = `${fahrenheitTemp}°F`; | ||
| tempValue.style.color = getFontColor(fahrenheitTemp); | ||
| landScape.textContent = getLandscape(fahrenheitTemp); |
There was a problem hiding this comment.
If we restructured our code to use the state object that was talked about inside of the reading we could actually move this logic into their respective functions. This would help with SRP and make our code easier to debug and maintain. Essentially, we would just replace the code here with something like state.temp = tempResponse.data.main.temp; and then we could just call our functions that handle updating our UI. In those functions we could have something like tempValue.textContent = ${state.temp}°F;.
| cityNameInput.addEventListener('input', (event) => { | ||
| updateCityName(event.target.value); | ||
| }); | ||
|
|
||
| cityNameReset.addEventListener('click', () => { | ||
| cityNameInput.value = ''; | ||
| updateCityName('Seattle'); | ||
| }); |
There was a problem hiding this comment.
These could also be moved into the document.addEventListener callback that I mentioned above.
No description provided.