search

MLH-Fellowship / prep-project-4.1.1

MLH Prep Project for Pod 4.1.1
https://mlh-prep-4-1-1.netlify.app/
MIT License
6 stars 13 forks source link

Map for selecting location #41

Open aniketbiswas21 opened 2 years ago

aniketbiswas21 commented 2 years ago

Fixes #5 and #21

aniketbiswas21 commented 2 years ago

@khattakdev Please take a look!

netlify[bot] commented 2 years ago

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

🔨 Explore the source changes: fcd614aeab01991c697be5ae4d2828947f9d24ef

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

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

atharmohammad commented 2 years ago

Looks good but i think location should be in a container as in this background i can't be able to see it also some bottom margin would look good location

Swatilekha-Roy commented 2 years ago

Agreed with @atharmohammad. Some margin-bottom would look really cool!

aniketbiswas21 commented 2 years ago

@atharmohammad @Swatilekha-Roy I have made the changes you requested. Please take a look!

atharmohammad commented 2 years ago

@aniketbiswas21 Looks good but i think shifting of App.js code to Home.js gonna cause lot of conflicts in other awaiting PR to get merged. it would be good if other PR's get merged first and then conflict should be resolved in this PR and get it merged

aniketbiswas21 commented 2 years ago

@aniketbiswas21 Looks good but i think shifting of App.js code to Home.js gonna cause lot of conflicts in other awaiting PR to get merged. it would be good if other PR's get merged first and then conflict should be resolved in this PR and get it merged

I think it should be the other way around. Since the Home.js is currently updated with the main's App.js and every other PR would require only a small amount of resolving. But, if all the PRs get merged first, then it would be quite a daunting task to accommodate all the changes again in a single PR.

cc: @khattakdev

atharmohammad commented 2 years ago

@aniketbiswas21 Looks good but i think shifting of App.js code to Home.js gonna cause lot of conflicts in other awaiting PR to get merged. it would be good if other PR's get merged first and then conflict should be resolved in this PR and get it merged

I think it should be the other way around. Since the Home.js is currently updated with the main's App.js and every other PR would require only a small amount of resolving. But, if all the PRs get merged first, then it would be quite a daunting task to accommodate all the changes again in a single PR.

Well it's a mess because most of work is done directly in App.js instead of in seperate components so maybe it would be alright to merge this first as your PR contains some major work and we dont have enough time now :laughing:

0xPrimata commented 2 years ago

Hey! looking pretty good. I just wanted to ask. is the extreme-IP API call breaking the page? On your PR it is commented out. @aniketbiswas21

aniketbiswas21 commented 2 years ago

Hey! looking pretty good. I just wanted to ask. is the extreme-IP API call breaking the page? On your PR it is commented out. @aniketbiswas21

Yes. Actually, it returns an error "Failed to fetch" for some reason.

The same happens for me even if I use the current production build.

aniketbiswas21 commented 2 years ago

@khattakdev Please take a look!