C22 Phoenix- Amber Edwards & Tatyana Venanzi #17
Conversation
Also added an inital temp of 70 to HTML
…ed into text input bar, set "Seattle" as default city
Instead of using a series of if statements, the color and landscape strings were move to a list variable. This makes the code more concise and easier to maintain. Also made a function that combines the two functions.
| <span id="current-weather-text">Current weather in the lovely city of</span> | ||
| <span id="headerCityName" class="header-city-name"></span> |
There was a problem hiding this comment.
I suggest replacing at least the first span with static text with an element like h2 or p.
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
| <img id="weather-icon" src="" alt="weather-icon"> | ||
| </section> | ||
| <section id="wsTemp" class="weather-section"> | ||
| <h3>Temperature</h3> |
There was a problem hiding this comment.
In the MDN docs it's highly suggested to not skip headings:
A common navigation technique for users of screen reading software is jumping from heading to quickly determine the content of the page. Because of this, it is important to not skip one or more heading levels. Doing so may create confusion, as the person navigating this way may be left wondering where the missing heading is.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements#navigation
Here the suggestion would be to use h2 and style it as needed to match the smaller font size desired.
| <div class="input-container"> | ||
| <input type="text" id="cityNameInput"> | ||
| </div> | ||
| <div class="button-container"> |
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?
| if (temp === 49 && state.realTempValue<= temp){ | ||
| colorData.realTempValueColor = tempProperties[idx].color; | ||
| break; | ||
| }else if (state.realTempValue >= temp){ |
There was a problem hiding this comment.
Spacing around parentheses and braces is inconsistent across the file. It's a small thing, but every bit of consistency across our code make it easier and faster to read.
} else if (state.realTempValue >= temp) {We should have spaces between => operators and curly braces, as well.
| let data = {}; | ||
| for (const idx in tempProperties){ | ||
| let temp = tempProperties[idx].temp; | ||
| if (temp === 49 && state.gardenTempValue<= temp){ |
There was a problem hiding this comment.
It's important to leave spaces between comparison operators so it's easy to see the parts of the expression at a quick glance.
| if (temp === 49 && state.gardenTempValue<= temp){ | |
| if (temp === 49 && state.gardenTempValue <= temp){ |
| const increaseTemp = () =>{ | ||
| state.gardenTempValue += 1; | ||
| const temp = document.getElementById('tempValue'); | ||
| temp.textContent = state.gardenTempValue; | ||
| changeGardenTempValueColorAndLandscape(); | ||
| }; | ||
|
|
||
| const decreaseTemp = () =>{ | ||
| state.gardenTempValue -= 1; | ||
| const temp = document.getElementById('tempValue'); | ||
| temp.textContent = state.gardenTempValue; | ||
| changeGardenTempValueColorAndLandscape(); | ||
| }; |
There was a problem hiding this comment.
Since increaseTemp and decreaseTemp are very similar, another approach we could take is to combine them into 1 function:
const setTemp = (changeInTemp) => {
state.gardenTempValue += changeInTemp;
const temp = document.getElementById('tempValue');
temp.textContent = state.gardenTempValue;
changeGardenTempValueColorAndLandscape();
};Then where we register the handlers, we could call our new function with a parameter by doing so inside an anonymous function:
increaseTempButton.addEventListener('click', () => { setTemp(1) });
decreaseTempButton.addEventListener('click', () => { setTemp(-1) });| const getCityCoords = () =>{ | ||
| const city = state.name; | ||
| return axios | ||
| .get('https://ada-weather-report-proxy-server.onrender.com/location', {params:{q: city}}) | ||
| .then((response)=>{ | ||
| const results = { | ||
| cityLat:response.data[0].lat, | ||
| cityLon: response.data[0].lon | ||
| }; | ||
| return results; | ||
| }) | ||
| .catch((error) => HandleCityNameError()); | ||
| } | ||
|
|
||
| const getCityWeatherData = (coordObject) =>{ | ||
| return axios | ||
| .get ('https://ada-weather-report-proxy-server.onrender.com/weather', {params:{lat: coordObject.cityLat, lon: coordObject.cityLon}}) | ||
| .then((response)=>{ | ||
| const tempK = response.data.main.temp; | ||
| const tempF = (tempK - 273.15) * 1.8 + 32; | ||
| const weatherData ={ | ||
| realTempValue: parseInt(tempF.toFixed(0)), | ||
| weather: response.data.weather[0].main, | ||
| weatherIconCode: response.data.weather[0].icon, | ||
| weatherDescription: response.data.weather[0].description, | ||
| }; | ||
| return weatherData; | ||
| }) | ||
| .catch((error) => HandleCityNameError()); | ||
| }; | ||
|
|
||
|
|
||
| const getCityData = () => { | ||
| return getCityCoords() | ||
| .then( (coordList) => { | ||
| return getCityWeatherData(coordList); | ||
| }) | ||
| }; |
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.
|
|
||
| .left-side { | ||
| border: 8px solid white; | ||
| /* background-color: white; */ |
There was a problem hiding this comment.
Commented out code should be removed before opening PRs.
No description provided.