-
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
C17-Sharks-Raha #66
base: main
Are you sure you want to change the base?
C17-Sharks-Raha #66
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.
Nice work Raha! Project grade: 🟢
Key takeaways:
- Event listeners are our friend! They search for HTML elements to apply functions to instead of requiring us to invoke functions in our HTML files. We can clean up the HTML file by removing all function calls and replacing them with event listners in index.js. I provided an example of how in the PR.
- Always make sure you have a .gitignore file to add
.env
file to. Never push up your .env file to github! This leaves your API keys vulnerable for others to steal! venv/
is NOT needed for javascript projects, only for python projects. Be wary of what you push inside of PRs!
Other than that keep up the great work Raha!
LOCATION_KEY="pk.8e923bdd6caecdd41fb592621e6457ec" | ||
WEATHER_KEY="8ef694b082e1cf1d9e6407167bd6a7b5" |
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.
This is dangerous, your API keys are open to the public and vulnerable to strangers using your account! You need to create a .gitignore
file and add .env
to it in order to prevent local environment files from being pushed publicly.
let latitude; | ||
let longitude; | ||
|
||
const weatherData = axios.get('http://localhost:5001/location', { params: { q: 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 moving the url into a constant variable towards the top of this file to improve readability.
.catch((error) => { | ||
console.log('error'); | ||
}); | ||
}) | ||
.catch((error) => { | ||
console.log('error :('); | ||
}); |
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.
Nice error handling. Perhaps we can write more descriptive errors such as 'temperature not found' or 'location not found'. We could also display a div
containing the error in case it's useful to an end user.
city = 'Atlanta'; | ||
document.querySelector('#cityname').value = 'Atlanta'; | ||
const curWeatherHeader = document.getElementById('cityQuote'); | ||
curWeatherHeader.textContent = 'Hotlanta! The City of Sweet tea and Sunshine'; |
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.
☀️🫖
<button onclick="getCurrentTemp()"> Get Current Temperature</button> | ||
<br><br><br> | ||
<label id="temperature"></label> | ||
|
||
<br><br> | ||
<button onclick="increaseTemp()">Increase Temperature</button> | ||
<button onclick="decreaseTemp()">Decrease Temperature</button> | ||
</div> | ||
<div class="grid-item"> | ||
<label for="cityid">CITY</label> | ||
<br> | ||
<input type="text" placeholder="Enter a City" id="cityname"> | ||
<br> | ||
<button onclick="resetCity()"> Reset City </button> |
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.
We should try to avoid invoking functions in our HTML so that this file's sole responsibility it just to display UI (practicing separating of concerns). We can achieve this by setting event listeners on with elements with specific id's. Here is how we'd change this button to have an id, and how we would apply an event listener to that button in Javascript:
HTML
<button id="reset"> Reset City </button>
Javascript
const resetCity = function () {
city = 'Atlanta';
document.querySelector('#cityname').value = 'Atlanta';
const curWeatherHeader = document.getElementById('cityQuote');
curWeatherHeader.textContent = 'Hotlanta! The City of Sweet tea and Sunshine';
getCurrentTemp();
updatteLandBasedonTemp();
};
const registerEventHandlers = () => {
// This function will contain all the event listeners we add to elements in index.html
const resetCityButton = document.getElementById('reset');
resetCityButton.addEventListener('click', resetCity);
}
// This line executes registerEventHandlers immediately on load.
document.addEventListener('DOMContentLoaded', registerEventHandlers);
let city = 'Atlanta'; | ||
let temperature = 70; |
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 storing these "global" variables in an object. This simulates how 'state' is managed in most frontend frameworks and is a common pattern you'll see in industry.
const increaseTemp = () => { | ||
temperature += 1; | ||
newTemperature(); | ||
}; | ||
|
||
const decreaseTemp = () => { | ||
temperature -= 1; | ||
newTemperature(); | ||
}; |
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.
Notice how there's only one line of code that differs between these two functions. We could combine these two functions and identify the exact element that was clicked using event
data like so:
const changeTemp = (e) => {
const degreeCountContainer = document.getElementById("temperature");
if (e.target.id == 'temp-up') {
state.temp += 1;
}
else if (e.target.id == 'temp-down') {
state.temp -= 1;
}
newTemperature();
};
const registerEventHandlers = () => {
const tempUp = document.getElementById('temp-up');
tempUp.addEventListener('click', changeTemp);
const tempDown = document.getElementById('temp-down');
tempDown.addEventListener('click', changeTemp);
}
.then((response) => { | ||
latitude = response.data[0].lat; | ||
longitude = response.data[0].lon; | ||
axios | ||
.get('http://localhost:5001/weather', { | ||
params: { lat: latitude, lon: longitude }, | ||
}) | ||
.then((response) => { | ||
const kelvin = response.data.current.temp; | ||
const fahrenheit = (9 / 5) * (kelvin - 273) + 32; | ||
temperature = Math.round(fahrenheit); | ||
newTemperature(); | ||
}) |
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.
Good work performing the nested API call using the locationIQ API call and the weather API.
Also great work with the design of your weather report! The layout is really neat and I love the images you chose for the landscape and the sky!! I'd say my one feedback is to make the font a little bit bigger so it's easier to read the buttons and options. |
No description provided.