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

Updated Penn Events Tab but w/out Club Events (yet) #542

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

innocentoak
Copy link
Member

No description provided.

…t and end dates from backend API, new events API manager, and updated PennEventsView to populate events)
…ventsViewModel (grouping into college houses, engineering, wharton, penn today, venture lab, all)
…itch when in features grid), reason was due to nested navigationViews
…nd event type string to find coordinates for mapview
…e categorization, and using that as another way to query for location, also reordered order of querying and looks at event type first (college houses first)
…eating separate coordinate and region vars (finally)
Copy link
Member

@anli5005 anli5005 left a comment

Choose a reason for hiding this comment

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

Aside from a few nits, nice work!

Will defer to @JHawk0224 on anything brought up in the review

PennMobile/Events/Old Events/EventsAPI.swift Outdated Show resolved Hide resolved
PennMobile/Events/Models/MailComposerCoordinator.swift Outdated Show resolved Hide resolved
PennMobile/Events/Models/PennEvent.swift Outdated Show resolved Hide resolved
PennMobile/Events/Models/PennEventsAPIManager.swift Outdated Show resolved Hide resolved
PennMobile/Events/View Models/PennEventViewModel.swift Outdated Show resolved Hide resolved
PennMobile/Events/Views/PennEventsView.swift Outdated Show resolved Hide resolved
PennMobile/Events/Views/PennEventsView.swift Outdated Show resolved Hide resolved
PennMobile/Events/Views/PennEventsViewerView.swift Outdated Show resolved Hide resolved
Copy link
Member

@JHawk0224 JHawk0224 left a comment

Choose a reason for hiding this comment

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

Overall it looks really good!

PennMobile.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
PennMobile.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
PennMobile/Events/Models/PennEvent.swift Outdated Show resolved Hide resolved
PennMobile/Events/Models/PennEventsAPIManager.swift Outdated Show resolved Hide resolved
PennMobile/Events/Old Events/EventsAPI.swift Outdated Show resolved Hide resolved
PennMobile/Events/Views/PennEventCellView.swift Outdated Show resolved Hide resolved
PennMobile/Events/Views/PennEventCellView.swift Outdated Show resolved Hide resolved
PennMobile/Events/Views/PennEventsViewerView.swift Outdated Show resolved Hide resolved
PennMobile/Events/Views/PennEventsViewerView.swift Outdated Show resolved Hide resolved
Copy link
Member

@JHawk0224 JHawk0224 left a comment

Choose a reason for hiding this comment

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

Things todo:

  • Remove commented file
  • Combine Penn Coordinate with your location file
  • Look into making the eventType an enum if possible
  • Other unresolved comments

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

Successfully merging this pull request may close these issues.

3 participants