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

feat: locations for custom events #726

Merged
merged 12 commits into from
Nov 24, 2023
Merged

Conversation

ap0nia
Copy link
Collaborator

@ap0nia ap0nia commented Oct 10, 2023

Summary

A continuation of #586 . I'll be collaborating with @stevenguyukai to prepare all the features.

Features

  • Custom events can optionally be associated with a building on campus, this is captured via the building's ID in the custom event
  • The building selector has been extracted to a separate component
  • The custom event create/edit dialog allows users to select a building for the custom event

TODO

  • Figure out how to get it into the map routing (if this is desired)

@github-actions github-actions bot requested a review from EricPedley October 10, 2023 23:21
@stevenguyukai
Copy link
Contributor

stevenguyukai commented Oct 11, 2023

More TODO (ALL Finished)

1. Custom Calendar event

1
  • Showing Location info on the pop ups
  • And the Location on the pop up is clickable, so when click the building, it will open the map mark for that event

2. Rearrange Custom Event Dialog

2
  • Put the building searching box before the schedule selector.
  • Because for most the student, they need no change of the schedules. So let it just be a default. So put it to the bottom

3. ADDED CLASSES Location Info

3
  • Adding location information to the custom event under ADDED CLASSES

@ap0nia ap0nia removed the request for review from EricPedley October 11, 2023 00:45
@stevenguyukai
Copy link
Contributor

Update

1. Custom Event Marker

In the Map.tsx I added a big function getCustomEventPerBuilding() so it can return the mapping information for custom event.

2. Add custom event to the route

It is very hard to do so since the course and custom event type are very different. I decide to just let custom event marked as E on the map and will not go into the route.

image

3. Location link

The location for Custom event in ADDED CLASSES is also clickable now

image

TODO

1. Clean the Code

Make the code more concise

2. Fix problem

I found like adding custom event with building MSTB will result in no marker on the map. Maybe it is caused by the & letter in the name

@EricPedley
Copy link
Member

Can you convert this to a draft until it's ready for review? Just makes our PR list easier to track.

@ap0nia ap0nia marked this pull request as draft October 27, 2023 14:00
@stevenguyukai stevenguyukai marked this pull request as ready for review November 6, 2023 01:35
@MinhxNguyen7
Copy link
Member

Is this PR done? There seems to be another TODO.

I strongly think that it would make sense for it to work with routing.

@stevenguyukai
Copy link
Contributor

Is this PR done? There seems to be another TODO.

I strongly think that it would make sense for it to work with routing.

I finished the intended features and is waiting for review and merge

@KevinWu098
Copy link
Member

Is this PR done? There seems to be another TODO.
I strongly think that it would make sense for it to work with routing.

I finished the intended features and is waiting for review and merge

Screenshot 2023-11-13 at 10 57 31 PM

Unless I'm mistaken, the latest staging instance doesn't have the map routing? Clicking a location also leads to https://staging-726.antalmanac.com/map?location=0

@stevenguyukai
Copy link
Contributor

stevenguyukai commented Nov 14, 2023

Maybe because of the Conflicts? Yea, the instance is differ from my local

@stevenguyukai
Copy link
Contributor

@ap0nia Would you happen to know why the instance differs from my local deployment? And also why the tests failed. (For the newest instance I merged main, but it still act differently)

@stevenguyukai stevenguyukai self-assigned this Nov 17, 2023
Copy link
Member

@MinhxNguyen7 MinhxNguyen7 left a comment

Choose a reason for hiding this comment

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

It would be good if the routing worked, but I'm in favor of getting this out the door, then working on that.

Otherwise, LGTM!

MinhxNguyen7

This comment was marked as duplicate.

@stevenguyukai
Copy link
Contributor

It would be good if the routing worked, but I'm in favor of getting this out the door, then working on that.

Otherwise, LGTM!

OK i will come back to this problem after

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.

5 participants