Skip to content
This repository was archived by the owner on Aug 7, 2024. It is now read-only.

Conversation

dhruvilmehta
Copy link
Member

@dhruvilmehta dhruvilmehta commented Jun 11, 2023

Fixes Issue

fixes #4789

Changes proposed

First of all there are many changes I am proposing to successfully solve this issue.

  1. Issue [FEATURE] Add location to in-person events #4132 with Pull request Added location to all events (In-person && Remote) #4438 was closed by adding 4 location parameters(road, city, state, country) but to locate the event into Map we need 2 more parameters which is Latitude and Longitude so I have added that 2 parameters in the events section.
    For testing purposes I have added these 2 parameters in this event "data\TRohit20\events\2023-06-13-DevRelCon-London.json"
  2. Also, the pull request I mentioned was incomplete because though he had added the location parameters in the event, he had not made that changes into the mongoose model and hence the location was not going into database.
  3. I have made the necessary changes to store the location parameter into the mongoose model.
  4. I have fetched the events from "pages/map.js" and further passed that events to "components/Map.js"
  5. I have also added an event icon in the "public" folder to show in the map that there is an event. (see the screenshot added)
  6. Done necessary changes in "events" documentation.

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

Screenshot (42)
This is how the location parameters look like (i have added "lat" and "lon" into this to locate this in map)

Screenshot (46)

Screenshot (45)
As soon as we click into the event shown on map we can see the details of the event.

Note to reviewers

@dhruvilmehta dhruvilmehta requested a review from SaraJaoude as a code owner June 11, 2023 06:39
@github-actions github-actions bot added large Pull request with more than 30 changed lines waiting-for-reviewers ✍ chore: profile This label will be added on PR with request of Data json addition labels Jun 11, 2023
@eddiejaoude eddiejaoude changed the title Issue#4789 Community events to map Jun 11, 2023
Copy link
Member

@eddiejaoude eddiejaoude left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution 👍

Please can you remove the placard.png image file and use an icon that is already available in the project from React Icons https://react-icons.github.io/react-icons

@dhruvilmehta
Copy link
Member Author

We have to put a URL in the "iconURL" field and therefore putting React Component (react-icon) is not working there. Can you provide me a link to some image which you wish to use there ? We cannot put a React Component there.

@dhruvilmehta
Copy link
Member Author

What would you suggest to do? I think you should keep placard.png file as it looks cool on the Map.

@eddiejaoude
Copy link
Member

Sorry for my slow reply, I will take a look today

@dhruvilmehta
Copy link
Member Author

Done with necessary changes... You may review it now

@eddiejaoude
Copy link
Member

Maps had a refactor recently, so you have a conflicts, please can you resolve and also check there are not further improvements/changes you wish to make after that PR got merged #7653

@dhruvilmehta
Copy link
Member Author

Done with necessary changes

@SaraJaoude SaraJaoude added issue linked Pull Request has issue linked and removed ✍ chore: profile This label will be added on PR with request of Data json addition labels Jun 23, 2023
Copy link
Member

@dan-mba dan-mba left a comment

Choose a reason for hiding this comment

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

I spent some time debugging and found that the location API has a relatively low usage threshold, so calls to it need to be minimized. The code I suggested that worked for all the future events over several tries.

@dhruvilmehta dhruvilmehta force-pushed the issue#4789 branch 2 times, most recently from 8d6e6bd to 3f26dcf Compare June 24, 2023 14:23
@dhruvilmehta
Copy link
Member Author

Conflicts are solved now.

@SaraJaoude SaraJaoude removed the ✍ chore: profile This label will be added on PR with request of Data json addition label Jul 3, 2023
@eddiejaoude
Copy link
Member

eddiejaoude commented Jul 9, 2023

Items I am looking at: (I will keep updating this list as I work through the PR)

  • remove image for event and use icon, fix in commit c676b9f
    • does the event need a logo? future idea
    • if virtual use icon future idea
    • if in person use icon future idea
  • map to have toggle for users/events (do events need tags also?)
  • refactor query to be more readable
  • refactor the way location is done to remove duplicate with user location

Copy link
Member

@eddiejaoude eddiejaoude left a comment

Choose a reason for hiding this comment

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

Great work 👍

I will merge to a temporary branch and make the final tweaks as I can't make changes to your fork - you are welcome to collab with me @dhruvilmehta

@eddiejaoude eddiejaoude changed the base branch from main to fix-pr-7522 July 9, 2023 19:20
@eddiejaoude eddiejaoude merged commit 0af682f into EddieHubCommunity:fix-pr-7522 Jul 9, 2023
@eddiejaoude
Copy link
Member

PR where I will make further changes #8093

`https://nominatim.openstreetmap.org/?addressdetails=1&q=
${encodeURIComponent(provided)}&format=json&limit=1`
);
const coordinates = await location.json();
Copy link
Member

Choose a reason for hiding this comment

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

For events that are added by forms, how do they get their location?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are fetching coordinates with city, state and country details. As soon as we hit the api to load data into MongoDB, it takes the events with startDate>today or EndDate>today and then fetches coordinates with city, state and country details.
This is what is happening, but I have not taken a look at how forms are working.

Copy link
Member

Choose a reason for hiding this comment

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

Ok thanks 👍 Forms will write directly to the db so will skip the reload script - I will look into this as I refactor the location fetching with users location

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue linked Pull Request has issue linked large Pull request with more than 30 changed lines storybook tests waiting-for-reviewers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Add community events to the map
5 participants