Skip to content
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

Weather-app #427

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Weather-app #427

wants to merge 24 commits into from

Conversation

zoe-lindqvist
Copy link

@JennieDalgren JennieDalgren self-assigned this Oct 4, 2024
Copy link
Contributor

@JennieDalgren JennieDalgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job with this project!
I really like the extra effort with the geolocation. So nice!!!
You've met the requirements.

One tiny thing to change when you have time, to make the project not look broken, on small screen the search bar gets placed on top on the temperature.
image

Really good job with usage of include() method. Just be aware that if you have data that would be called for example smash then this would give you true weather.includes("ash") because ash is included in smash. So it is really important to understand the data you are working with. for example if the value will be exactly "ash" "haze" "fog" etc you can also do the checks like:

if (weather === "ash") etc...

Just a clarfication.

Another tiny UX detail, if you wrap your input and button for the search bar into a form element and put the submit event on the form instead, the user can use both the button or the enter key to search.

Good structure of the code and good use of comments.
Nice job with error handling too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants