SL c18- Cristal B. & Diana S. #77
Conversation
There was a problem hiding this comment.
Looking good. Try to think about how to refactor the API call helpers so that they're a little more independent and reusable. I have some suggestions in the comments. There's also quite a bit of code based on similar if/else if statements that we could think about making more data-driven, simplifying our code, and reducing the likelihood of typos, so check my comments for more details. And rather than having element lookup and event registration code scattered throughout the script, prefer to group related operations together, and register them to run on page load with the DOMContentLoaded event.
| @@ -0,0 +1,75 @@ | |||
|
|
|||
| * { | |||
| box-sizing: border-box; | |||
There was a problem hiding this comment.
👍 border-box breaks my brain much less than the default content-box rules.
|
|
||
| body { | ||
| /* margin: 0; */ | ||
| display: flex; |
There was a problem hiding this comment.
Nice use of flex along with specified element widths to achieve your four-square layout.
|
|
||
| <h1 id="change-city" class="diff-city">🌳 Weather for the City of <span id="cityInput">Seattle</span> 🍃</h1> | ||
|
|
||
| <script src="./node_modules/axios/dist/axios.min.js"></script> |
There was a problem hiding this comment.
Prefer to keep you script imports together. We should put this down by the script tag importing our index.js.
| <span id="down"><button>🔽</button></span> | ||
| <button id="currentWeather">Get real time weather</button> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
This is an extra closing div which breaks validation.
| @@ -1,12 +1,59 @@ | |||
|
|
|||
There was a problem hiding this comment.
Be sure to validate your html when possible. The errors/warnings can help us write cleaner HTML. Even if our markup looks fine in the browser we're using, there's no guarantee invalid HTML will do so on other browsers, and it could even break in the same browser since there are no layout guarantees made for invalid HTML.
| state.city = newCity; | ||
| displayCity(); |
There was a problem hiding this comment.
I like how this is set up to update the state value for the city, and then use the displayCity function, whose responsibility is to update the UI to reflect the current city state. Consider extracting these lines into a helper setCity(newCity) that you could also use in the reset handler.
| const resetText = () => { | ||
| const newCity = document.getElementById('newCity'); | ||
| newCity.value = 'Seattle'; | ||
| updateCity(); |
There was a problem hiding this comment.
updateCity expects to be registered as an event handler (it has a parameter to receive event details). Calling it here without event information leads to an error when trying to update the city in the UI. If we had a setCity helper as described above, we could use that instead, passing in the reset default city value.
| const displayEmojis = () => { | ||
| let numColor = 'red'; | ||
| let emojisBelow = '🌵__🐍_🦂_🌵🌴__🐍_🏜_🦂'; | ||
| if (state.temp > 80) { |
There was a problem hiding this comment.
Notice the repetition in these if/else if blocks. Code like this tends to be finicky, since humans tend to make easily overlooked typos that can be hard to track down. Consider a data structure and accompanying code similar to the following:
const tempColors = [
[80, {ground: 'ground emojis', color: 'temp color'}],
[70, {ground: 'ground emojis', color: 'temp color'}],
[60, {ground: 'ground emojis', color: 'temp color'}],
[50, {ground: 'ground emojis', color: 'temp color'}],
[null, {ground: 'ground emojis', color: 'temp color'}],
];
const getSettingsForTemp = (temp) => {
for (const [boundaryTemp, tempSettings] of tempColors) {
if (boundaryTemp === null || temp > boundaryTemp) {
return tempSettings;
}
}
};Looking for repetition in the structure of our code and refactoring it to be captured in a data structure instead can make our code more flexible (behavior can be changed solely by changing data) while simplifying the code working with the data.
If we had this function, how could we simplify the logic here in displayEmojis?
| newEmojis.textContent = emojisBelow; | ||
|
|
||
| const temperature = document.getElementById('tempNum'); | ||
| temperature.className = numColor; |
There was a problem hiding this comment.
Nice approach of setting a css class with the style details rather than setting the style properties in the code here itself.
| temperature.textContent = String(state.temp); | ||
| }; | ||
|
|
||
| const updateSky = () => { |
There was a problem hiding this comment.
The comments about splitting the function responsibility for temp/color would also apply to the sky function here. To think about how we could make this more data-driven, consider what changes we might make to this function if we had a data structure resembling:
const skySettings = {
clouds: 'clouds emoji',
sunshine: 'sunshine emoji',
rain: 'rain emoji',
snow: 'snow emoji',
wind: 'wind emoji',
};
No description provided.