-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Huma H./Sharks/C17 #86
base: main
Are you sure you want to change the base?
Conversation
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.
Great job! Your code was easy to read and I was able to do all of the required tasks in your weather app!
Please let me know if you have any questions about my comments
🟢 for weather-report!
</section> | ||
</main> | ||
<script src="src/index.js"></script> | ||
<script src="./node_modules/axios/dist/axios.min.js"></script> |
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.
Clearly written, semantic HTML 👍
lat: '', | ||
lon: '', |
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.
You could also hard code the lat/lon for Orlando here instead of setting them to empty strings
|
||
const displayRealTemp = (tempF) => { | ||
const tempValueElement = document.getElementById('tempValue'); | ||
// tempValueElement.textContent = state.tempF; |
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.
Remove unused commented out code to avoid introducing bugs if it accidentally gets uncommented
}) | ||
.then((response) => { | ||
console.log(response); | ||
// const tempValueK = response.data.current.temp; |
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.
Remove unused commented out code
const getLatLon = () => { | ||
const inputCityElement = document.getElementById('inputCity').value; | ||
axios | ||
.get('http://127.0.0.1:5000/location', { params: { q: state.city } }) |
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.
Consider using a constant variable like LOCATION_URL instead of string literal here.
"Constants free the programmer from having to remember what each literal should be. Often values that stay constant throughout the program have a business meaning. If there are several such values, the programmer can define them all in the beginning of the program and then work with the easier-to-remember constant names." From: https://www.diffen.com/difference/Constant_vs_Literal
if (state.tempValue <= 59) { | ||
landscapeElement.textContent = '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲'; | ||
state.background = "url('../assets/winter.jpg')"; | ||
document.querySelector('html').style.backgroundImage = | ||
"url('../assets/winter.jpg')"; | ||
} else if (60 <= state.tempValue && state.tempValue <= 69) { | ||
landscapeElement.textContent = '🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃'; | ||
state.background = "url('../assets/autumn.jpg')"; | ||
document.querySelector('html').style.backgroundImage = | ||
"url('../assets/autumn.jpg')"; | ||
} else if (70 <= state.tempValue && state.tempValue <= 79) { | ||
landscapeElement.textContent = '🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷'; | ||
state.background = "url('../assets/summer.jpg')"; | ||
document.querySelector('html').style.backgroundImage = | ||
"url('../assets/summer.jpg')"; | ||
} else if (80 <= state.tempValue) { | ||
landscapeElement.textContent = '🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂'; | ||
// state.background = url('../assets/desert.jpg'); | ||
document.querySelector('html').style.backgroundImage = | ||
"url('../assets/desert.jpg')"; | ||
// document.html.style.backgroundImage = "url('../assets/desert.jpg')"; | ||
} |
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.
Check out the logic here, see how similar the branches are of the conditional statement. We look for whether the temperature is within a range, and pick a color accordingly. What if we had a list of objects that we could iterate through to find the values. We could set up something like
const TEMP_LANDSCAPE = [
{ upperBound: 59, landscape: '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲', background: 'url('../assets/winter.jpg')'},
{ upperBound: 69, landscape: '🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃', background: 'url('../assets/autumn.jpg')'}
];
Then your method would iterate through TEMP_LANDSCAPE
and find the first record that has an upper bound higher than our temperature, then use it as the source of picking the landscape which would help make the function a bit more concise.
This could accommodate a scenario where you might have even more landscapes in the future, which would prevent your conditional statement from being a really long block of if/elif/elif and so on
if (state.tempValue <= 59) { | ||
landscapeElement.textContent = '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲'; | ||
state.background = "url('../assets/winter.jpg')"; | ||
document.querySelector('html').style.backgroundImage = |
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.
Instead of accessing document.querySelector('html').style.backgroundImage
in each if/elif block, you could create a variable on line 106:
const htmlElement = document.querySelector('html').style.backgroundImage;
Then you could use htmlElement
in each block instead of the long statement
if (skyDropdownElement.value === 'sunny') { | ||
state.skyscape = '☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️'; | ||
skyscapeElement.textContent = state.skyscape; | ||
} else if (skyDropdownElement.value === 'cloudy') { | ||
state.skyscape = '☁️☁️☁️☁️☁️☁️☁️☁️☁️☁️☁️🌦☁️☁️☁️☁️☁️☁️☁️☁️☁️'; | ||
skyscapeElement.textContent = state.skyscape; | ||
} else if (skyDropdownElement.value === 'rainy') { | ||
state.skyscape = '🌧🌈⛈🌧💧🌧🌦🌧💧🌧⛈🌈🌧'; | ||
skyscapeElement.textContent = state.skyscape; | ||
} else { | ||
state.skyscape = '🌨❄️🌨🌨❄️❄️🌨❄️❄️🌨❄️❄️🌨🌨❄️🌨'; | ||
skyscapeElement.textContent = state.skyscape; |
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.
Similar to my comment in landscapeChangeAction
you could create a constant list of objects SKYSCAPE
and iterate over it to select the right emojis for the sky.
SKYSCAPE = [
{sky: 'sunny', skyscape: '☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️☀️'}
]
// skyscapeElement.textContent = state.skyscape; | ||
// skyscapeElement === state.skyscape.textContent; | ||
}; | ||
|
||
// result.textContent = `${updateSkyAction.target.value}`; | ||
// state.skyscape = `${updateSkyAction.target.value}`; | ||
// `string interpolation | ||
// ${} this will get value | ||
// Function to display city letter by letter as inputting |
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.
Remove
/* <label>Choose an ice cream flavor: | ||
<select class="ice-cream" name="ice-cream"> | ||
<option value="">Select One …</option> | ||
<option value="chocolate">Chocolate</option> | ||
<option value="sardine">Sardine</option> | ||
<option value="vanilla">Vanilla</option> | ||
</select> | ||
</label> | ||
|
||
<div class="result"></div> | ||
|
||
JavaScript | ||
const selectElement = document.querySelector('.ice-cream'); | ||
|
||
selectElement.addEventListener('change', (event) => { | ||
const result = document.querySelector('.result'); | ||
result.textContent = `You like ${event.target.value}`; | ||
}); */ | ||
|
||
// const arrowEx = () => { | ||
// // state.arrow += "⬆" | ||
// const upTempElement = document.getElementById('upTemp'); | ||
// upTempElement.textContent += '⬆'; | ||
// }; | ||
|
||
// upTempElement.addEventListener('click', arrowEx); |
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.
Be sure to clean up your code to remove comments/print statements, which makes your code more readable.
No description provided.