Tigist/zoisite #58
Tigist/zoisite #58
Conversation
dispalys city changes on input
weather and background updatein real time with api call
changes sky background with sky condition option
adds reset button
| @@ -0,0 +1,164 @@ | |||
| h2 { | |||
There was a problem hiding this comment.
I saw that index.css is linked in index.html, but don't see reference to this file in the project. Is this needed or can it be removed?
| <div id="temp--title">Temp</div> | ||
| <div id="temp--controls"> | ||
| <button id="decrease-t">down</button> | ||
| <div id="temperature">70</div> | ||
| <button id="increase-t">up</button> |
There was a problem hiding this comment.
Prefer that these elements be nested in a semantic html element, maybe something like a section tag. Additionally, I think that the "temp" and "70" text should be heading tags or something more semantic than just a div tag, which has no meaning but is often used for styling.
| </div> | ||
| </div> | ||
| <div id="buttom--left--sky"> | ||
| <div id="sky--title">Sky</div> |
There was a problem hiding this comment.
Prefer "Sky" text to be a heading tag or something more semantic than a div.
Also "Sky" element and the select element with options elements could all be contained in a section tag to bring more structure to the HTML. Someone not navigating your site visually (maybe using a screen reader) would understand that "Sky" and the drop down menu are grouped together.
| </div> | ||
| </div> | ||
| <div id="buttom-left--city"> | ||
| <div id="city--title">City</div> |
| <form action="/url" method="GET"> | ||
| <input id="city-input" type="text" name="city_name" placeholder="Please enter your city" required /> | ||
| <button id="submit-button" type="submit">Submit</button> | ||
| </form> | ||
| <div id="reset-button"> | ||
| <input type="reset" value="Reset" /> | ||
| </div> |
There was a problem hiding this comment.
The form, buttons and the "City" heading should all be grouped together with an element like a section element to convey to users that these elements are logically grouped together.
| const increaseTemp = () => { | ||
| state.currentTemp++; | ||
| changesTempDisplay() | ||
| }; | ||
|
|
||
| const decreaseTemp = () => { | ||
| state.currentTemp--; | ||
| changesTempDisplay() | ||
| }; |
There was a problem hiding this comment.
notice that these two methods do essentially the same thing but the difference is ++ or --.
How could you combine the logic of these two methods into one event handler called handleTempChange()?
What if you pass in the event object as an argument and you check if the event's target has an id increase-t or decrease-t and depending on the id then you could ++ or --
const handleTempChange = (event) => {
if (event.target.id === 'increase-t') {
state.currentTemp++;
} else {
state.currentTemp--;
}
changesTempDisplay();
};| console.log('skyBackground') | ||
| const condition = state.skyCondition.value | ||
| if(condition == 'sunny') { | ||
| console.log('sunny') |
There was a problem hiding this comment.
Remove the debugging print statements on line 30 and 33 before you open a pull request for your code to be reviewed
| state.skyImg.style.backgroundImage = 'url(assets/sky/kyaw-zay-ya-8vHAfhWoqEQ-unsplash.jpg)'; | ||
| } else if(condition == 'rainy') { | ||
| state.skyImg.style.backgroundImage = 'url(assets/sky/eutah-mizushima-F-t5EpfQNpk-unsplash.jpg)'; | ||
| } else if(condition == 'snowy'){ | ||
| state.skyImg.style.backgroundImage = 'url(assets/sky/fabrizio-conti-Mbm0WnJ5emc-unsplash.jpg)'; | ||
| } else if(condition == 'cloudy') { | ||
| state.skyImg.style.backgroundImage = 'url(assets/sky/daoudi-aissa-Pe1Ol9oLc4o-unsplash.jpg)'; |
There was a problem hiding this comment.
Prefer that the paths for each picture be referenced by constant variables which would keep this code more concise. It's also just good practice to reference strings with descriptive variable names.
const CLOUDY_IMG = url(assets/sky/daoudi-aissa-Pe1Ol9oLc4o-unsplash.jpg)';
state.skyImg.style.backgroundImage = CLOUDY_IMG;| state.skyImg.style.backgroundImage = 'url(assets/sky/daoudi-aissa-Pe1Ol9oLc4o-unsplash.jpg)'; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Nitpick - indentation is off, line 43 should be unindented one level to the left
| } else { | ||
| state.backgroundColor.style.backgroundColor = 'teal'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Another way to format this method would be to have an object that has temp values as keys and then you could have values be a list and the list could hold background color and background image, like:
const 80_DEGREE_IMG = 'url(assets/landscape/cody-doherty-AeqlmVWtzFA-unsplash.jpg)';
const 70_DEGREE_IMG = 'url(assets/landscape/alexandr-hovhannisyan-RkOtjbPuHZw-unsplash.jpg)';
const tempDisplayObject = {80: ['red', 80_DEGREE_IMG], 70: ['orange', 70_DEGREE_IMG]} When you have an object, you don't need to do if/else if/else if/ else which can introduce bugs and also adds complexity to the method. If you have an object, than you can iterate over the object and check the keys with state.currentTemp and then get the correct values to set backgroundColor and backgroundImage
No description provided.