Tigist/zoisite #58
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,64 @@ | ||
| <!DOCTYPE html> | ||
| <html lang="en"> | ||
|
|
||
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <meta http-equiv="X-UA-Compatible" content="IE=edge" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>Weather Report</title> | ||
| <link rel="preconnect" href="https://fonts.gstatic.com" /> | ||
| <link href="https://fonts.googleapis.com/css2?family=Rubik&display=swap" rel="stylesheet" /> | ||
| <link rel="stylesheet" href="styles/index.css" /> | ||
| </head> | ||
|
|
||
| <body> | ||
| <main> | ||
| <div id="top"> | ||
| <h1>Weather Report</h1> | ||
| </div> | ||
| <div id="buttom"> | ||
| <div id="buttom--left"> | ||
| <div id="buttom--left--temp"> | ||
| <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> | ||
| </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 id="sky--controls"> | ||
| <select name="sky-options" id="sky--options"> | ||
| <option value="rainy">Rainy</option> | ||
| <option value="sunny">Sunny</option> | ||
| <option value="snowy">Snowy</option> | ||
| <option value="cloudy">Cloudy</option> | ||
| </select> | ||
| </div> | ||
| </div> | ||
| <div id="buttom-left--city"> | ||
| <div id="city--title">City</div> | ||
|
|
||
| <div id="city--controls"> | ||
| <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> | ||
|
Comment on lines
+43
to
+49
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. |
||
| </div> | ||
| </div> | ||
| </div> | ||
| <div id="buttom--right"> | ||
| <div id="city-display"> | ||
| <div id="city-display--name">Current City</div> | ||
|
|
||
| </div> | ||
| </div> | ||
| </div> | ||
| </main> | ||
| <script src="./src/index.js"></script> | ||
| <script src="./node_modules/axios/dist/axios.min.js"></script> | ||
| </body> | ||
|
|
||
| </html> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| const state = { | ||
| currentTemp: 70, | ||
| backgroundColor: document.getElementById('buttom--left'), | ||
| landscapeImg: document.getElementById('buttom--right'), | ||
| cityInput: document.getElementById('city-input'), | ||
| location: null, | ||
| lat: null, | ||
| lon: null, | ||
| skyCondition: document.getElementById('sky--options'), | ||
|
There was a problem hiding this comment. Name variables to describe what the element is without needing to go refer back to HTML. Something like skyConditionMenu could work here |
||
| skyImg: document.getElementById('top') | ||
| } | ||
| // changes the temp on the page | ||
| const changesTempDisplay = () => { | ||
| const tempControl = document.getElementById('temperature'); | ||
| tempControl.textContent = `${state.currentTemp}` | ||
| } | ||
| // CHANGES TEMP | ||
| const increaseTemp = () => { | ||
| state.currentTemp++; | ||
| changesTempDisplay() | ||
| }; | ||
|
|
||
| const decreaseTemp = () => { | ||
| state.currentTemp--; | ||
| changesTempDisplay() | ||
| }; | ||
|
Comment on lines
+18
to
+26
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 const handleTempChange = (event) => {
if (event.target.id === 'increase-t') {
state.currentTemp++;
} else {
state.currentTemp--;
}
changesTempDisplay();
}; |
||
|
|
||
| // sky background change with sky option | ||
| const skyBackground = () => { | ||
| 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)'; | ||
|
Comment on lines
+34
to
+40
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; |
||
| } | ||
|
|
||
| } | ||
|
There was a problem hiding this comment. Nitpick - indentation is off, line 43 should be unindented one level to the left |
||
|
|
||
| // CHANGE BACKGROND COLOR AND IMAGE WITH TEMP CHANGE | ||
| const changeBackgrounds = () => { | ||
| if (state.currentTemp >= 80) { | ||
| state.backgroundColor.style.backgroundColor = 'red'; | ||
| state.landscapeImg.style.backgroundImage = 'url(assets/landscape/cody-doherty-AeqlmVWtzFA-unsplash.jpg)'; | ||
| } else if (state.currentTemp >= 70) { | ||
| state.backgroundColor.style.backgroundColor = 'orange'; | ||
| state.landscapeImg.style.backgroundImage = 'url(assets/landscape/alexandr-hovhannisyan-RkOtjbPuHZw-unsplash.jpg)'; | ||
| } else if (state.currentTemp >= 60) { | ||
| state.backgroundColor.style.backgroundColor = 'yellow'; | ||
| state.landscapeImg.style.backgroundImage = 'url(assets/landscape/andrew-neel-a_K7R1kugUE-unsplash.jpg)'; | ||
| } else if (state.currentTemp >= 50) { | ||
| state.backgroundColor.style.backgroundColor = 'green'; | ||
| state.landscapeImg.style.backgroundImage = 'url(assets/landscape/joel-jasmin-forestbird-4S3iMBIappo-unsplash.jpg)'; | ||
| } 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 |
||
|
|
||
| // DISPLAY CITY CHANGES ON INPUT | ||
| const updateCity = () => { | ||
| document.getElementById('city-display--name').textContent = state.cityInput.value | ||
| } | ||
|
|
||
| // reset city | ||
| const resetCity = () => { | ||
| state.cityInput.value = ""; | ||
| updateCity(); | ||
| } | ||
|
|
||
| // calling API | ||
| // Convert from Kel to Fahren | ||
| const handleRealTemp = async (kelvin) => { | ||
|
There was a problem hiding this comment. Do we need the This method converts kelvin to Fahrenheit and then calls changeTempDisplay() which is not an async method itself. |
||
| state.currentTemp = Math.floor((kelvin - 273.15) * 9/5 + 32 ); | ||
| changesTempDisplay() | ||
| }; | ||
|
|
||
| // activated by submit button | ||
| const submitLocationInput = () => { | ||
| state.location = state.cityInput.value; | ||
| getLatLon(); | ||
| } | ||
|
|
||
| // API CALLS | ||
| // loaction api | ||
| const getLatLon = () => { | ||
|
There was a problem hiding this comment. This comment applies to both methods that make an asynchronous API call: Usually a pattern when using promises is to return the promise chain like: return axios
.get(someUrl)
.then()
.catch()You've used promises in a way that you call other helper methods in your .then block so the functionality of your project works as expected even though you don't return the promise chain (see lines 91 and 110). There may be times when you aren't able to call other helper methods in .then() and you will need to return the promise chain so I wanted to call this to your attention. See slide 15 from our "Calling APIs" presentation for ore details. |
||
| axios.get('http://127.0.0.1:5000/location', { | ||
|
There was a problem hiding this comment. Prefer to put the URL in a constant variable Maybe something like Then on line 91, you can call: axios.get(`${BASE_URL}/location`) |
||
| params: { | ||
| q: state.location, | ||
| }, | ||
| }) | ||
| .then((response) => { | ||
| state.lon = response.data[0].lon; | ||
| state.lat = response.data[0].lat; | ||
| }) | ||
| .then(() => { | ||
| getWeather() | ||
| }) | ||
| .catch((error) => { | ||
| console.log(error) | ||
| }) | ||
| } | ||
|
|
||
| // weather api call | ||
| const getWeather = () => { | ||
| axios.get('http://127.0.0.1:5000/weather', { | ||
|
There was a problem hiding this comment. Prefer to have URL stored in a constant variable |
||
| params: { | ||
| lat: state.lat, | ||
| lon: state.lon, | ||
| }, | ||
| }) | ||
| .then((response) => { | ||
| kelvin = response.data.main.temp; | ||
| handleRealTemp(kelvin); | ||
| }) | ||
| .then(() => { | ||
| changeBackgrounds() | ||
|
There was a problem hiding this comment. Could you call .then((response) => {
kelvin = response.data.main.temp;
handleRealTemp(kelvin);
changeBackgrounds()
})
.catch((error) =>{
console.log(error)
})
// and delete lines 120-122 |
||
| }) | ||
| .catch((error) =>{ | ||
| console.log(error) | ||
| }) | ||
| } | ||
|
|
||
| // REGISTER EVENT LISTENER | ||
| const registerEventHandler = () => { | ||
| const tempDecrease = document.getElementById('decrease-t'); | ||
|
There was a problem hiding this comment. I can't tell from the name of the variable or the selector used what kind of element Also, in your state object has a couple of key-value pairs that are elements. Could you also add the elements in registerEventHandler into the state object? Right now you have some elements in object and some elements defined in a method, it would be nice to put them all in one place to keep the code consistent and organized. If you add your elements in the state object then you can delete line 130 and then directly access the element on line 131 like: |
||
| tempDecrease.addEventListener('click', decreaseTemp); | ||
| tempDecrease.addEventListener('click', changeBackgrounds); | ||
|
|
||
| const tempIncrease = document.getElementById('increase-t'); | ||
|
There was a problem hiding this comment. Same comment as above, consider renaming this variable to Would be good to add all the elements in registerEventHandler in the state object. |
||
| tempIncrease.addEventListener('click', increaseTemp); | ||
| tempIncrease.addEventListener('click', changeBackgrounds); | ||
|
|
||
| const inputCity = document.getElementById('city-input'); | ||
| inputCity.addEventListener('keyup', updateCity); | ||
|
|
||
| const submitButton = document.getElementById('submit-button'); | ||
| submitButton.addEventListener('click', submitLocationInput); | ||
|
|
||
| state.skyCondition.addEventListener('change', skyBackground); | ||
|
|
||
| const resetButton = document.getElementById('reset-button'); | ||
| resetButton.addEventListener('click', resetCity); | ||
|
|
||
| } | ||
|
|
||
| document.addEventListener('DOMContentLoaded', registerEventHandler) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.