MLH-Fellowship / prep-project-4.1.4

MLH Prep Project for Pod 4.1.4
https://mlh-prep-4-1-4.netlify.app/
MIT License
2 stars 12 forks source link

Fix Geolocation Error #33

Closed Pratyush1606 closed 2 years ago

Pratyush1606 commented 2 years ago

Fixes #32

Actually, in Pull Request #17, the geolocation is setting city and then using that value for displaying the corresponding weather. But in the merged version, cityCoordinates is being used to get the result.

In #17, fetch( "https://api.openweathermap.org/data/2.5/weather?q=" + city + "&units=metric" + "&appid=" + process.env.REACT_APP_APIKEY )

In the current main version, fetch( "https://api.openweathermap.org/data/2.5/weather?lat=" + cityCoordinates.lat + "&lon=" + cityCoordinates.lon + "&units=metric" + "&appid=" + process.env.REACT_APP_APIKEY )

netlify[bot] commented 2 years ago

✔️ Deploy Preview for mlh-prep-4-1-4 ready!

🔨 Explore the source changes: 49b719a3eadd4d3882377c662f67e8f045ec1394

🔍 Inspect the deploy log: https://app.netlify.com/sites/mlh-prep-4-1-4/deploys/61a0f4d63155a70007bff2f5

😎 Browse the preview: https://deploy-preview-33--mlh-prep-4-1-4.netlify.app/

shravani05 commented 2 years ago

@Pratyush1606 LGTM

NoahCardoza commented 2 years ago

It seems like this feature it's working for me on macOS Chrome:

image

Pratyush1606 commented 2 years ago

@NoahCardoza Are you talking about the preview of this pr or the main one?

NoahCardoza commented 2 years ago

@NoahCardoza Are you talking about the preview of this pr or the main one?

Maybe I'm not clear on what this PR does. I expected it would make sure the city name in the search bar matched the name in the card:

image

In this image the card says "San Jose" but the search bar says "New York".

However, after rereading your description, maybe that's wasn't within the scope of your PR?

shravani05 commented 2 years ago

@NoahCardoza yes right, this pr just detects location using geolocation api

NoahCardoza commented 2 years ago

LGTM, just a small merge conflict.

Pratyush1606 commented 2 years ago

@NoahCardoza Resolved the conflict :)