C22 - Kristina and Marjana #6
Conversation
| <span>For the lovely city of | ||
| <span id="headerCityName" class="header__city-name"> </span></span> |
There was a problem hiding this comment.
I suggest using something like a h2 or a p element around the inner span here.
span is much like div where it does not inherently have semantic meaning, so we want to reserve it for places where we need an inline element to wrap something for styling-purposes only. More info: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/span
| <section class="temperature__section"> | ||
| <h2>Temperature</h2> | ||
| <div class="temperature__content"> | ||
| <div class="temperature__controls"> |
There was a problem hiding this comment.
There are some divs across the HTML like this one that I might suggest replacing with section or article elements because it looks they are meaningfully sectioning content. That way we only leave divs where they are wrapping content for styling purposes.
I suggest taking a look at each div across the file and asking: if it is doing more work than styling, is there any semantic element that could be a better fit?
| } | ||
|
|
||
| // WAVE 2 | ||
| const updateTemp = (increment) => { |
There was a problem hiding this comment.
Great use of a single function for both buttons!
| } else if (skySelect.value == 'sunny') { | ||
| sky.textContent = `☁️ ☁️ ☁️ ☀️ ☁️ ☁️`; | ||
| gardenSection.classList.add('sunny'); | ||
| } else if (skySelect.value == 'cloudy') { | ||
| sky.textContent =`☁️☁️ ☁️ ☁️☁️ ☁️ 🌤 ☁️ ☁️☁️`; | ||
| gardenSection.classList.add('cloudy'); | ||
| } else if (skySelect.value == 'rainy') { | ||
| sky.textContent = `🌧🌈⛈🌧🌧💧⛈🌧🌦🌧💧🌧🌧`; | ||
| gardenSection.classList.add('rainy'); | ||
| } else if (skySelect.value == 'snowy') { | ||
| sky.textContent = `🌨❄️🌨🌨❄️❄️🌨❄️🌨❄️❄️🌨🌨`; | ||
| gardenSection.classList.add('snowy'); | ||
| } |
There was a problem hiding this comment.
When we have large if-statements like this where each if-block performs the same operations with slightly different values, it can be worth making a helper function that handles the updates to reduce places we're repeating code and could accumulate small typos. With the helper function, each of the ifs here would call that function passing the desired parameters rather than setting attributes themselves.
| const query = headerCityName.textContent; | ||
| const temp = await findLatitudeAndLongitude(query); | ||
| let realTimeTemp = Math.round(temp); | ||
|
|
||
| if (realTimeTemp !== null) { | ||
| tempValue.textContent = realTimeTemp; | ||
| updateTempColor(realTimeTemp); | ||
| } else { | ||
| tempValue.textContent = 'cannot find temperature'; | ||
| } |
There was a problem hiding this comment.
Is there a reason to keep this an anonymous function or could we move it over to a named helper function to reduce clutter in this function focused on registering the event handlers?
|
|
||
| const latitude = response.data[0].lat; | ||
| const longitude = response.data[0].lon; | ||
| console.log('success in findLatitudeAndLongitude!', 'latitude:', latitude, 'longitude:', longitude); |
There was a problem hiding this comment.
We should remove console.log statements from our Javascript before submission just like with print statements in our Python code.
| const longitude = response.data[0].lon; | ||
| console.log('success in findLatitudeAndLongitude!', 'latitude:', latitude, 'longitude:', longitude); | ||
|
|
||
| return await findWeather(latitude, longitude); |
There was a problem hiding this comment.
This function is not returning what its name suggests. It's called findLatitudeAndLongitude but instead of returning the latitude and longitude and letting the original caller do something with those values, the function feeds the latitude and longitude into another function findWeather and returns whatever that function gives back.
This is a pattern that is likely to cause confusion for folks reading through the code or working in the project. These two functions are now tightly coupled and we lose some of the benefit of having them separated out into their own named functions.
- How could we restructure how we use these two functions to make them better hold to the single responsibility principle and have the actions they take better reflect their names?
| } | ||
|
|
||
| // WAVE 6 | ||
| const resetCityName = () => { |
There was a problem hiding this comment.
Really nice use of helper functions to divide up the actions we want to take into smaller portions that are easier to read and test.
| display: flex; | ||
| flex-direction: row; | ||
| justify-content: space-around; | ||
| /* justify-content: center; */ |
There was a problem hiding this comment.
Commented out code should be removed before opening PRs.
| }); | ||
|
|
||
| console.log('success in findWeather!', response.data); | ||
| return response.data.main.temp; |
There was a problem hiding this comment.
By the OpenWeather documentation, the temp value is being returned in Kelvin units and should be converted to Fahrenheit if we want it to work with the temperature changes of the weather garden we created.
No description provided.