-
Notifications
You must be signed in to change notification settings - Fork 7
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
Angular v17 Upgrade: Events Feature Refactor #462
Angular v17 Upgrade: Events Feature Refactor #462
Conversation
frontend/src/app/organization/organization-admin/organization-admin.component.ts
Show resolved
Hide resolved
frontend/src/app/organization/organization-details/organization-details.component.ts
Outdated
Show resolved
Hide resolved
frontend/src/app/organization/organization-details/organization-details.component.html
Outdated
Show resolved
Hide resolved
frontend/src/app/organization/organization-page/organization-page.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: The problem is that resolvers run before canActivate guards in Angular, so there was no way to use the resolver's event data in the guard.
Did you try accessing the data
of route
to access the resolved data here:
export const eventEditorGuard: CanActivateFn = (route, _) => { |
Looking great! Pointed out a few places where I think things could be tidied or some open questions existed that are worth moving through before merging in.
Thank you for the review @KrisJordan! I fixed most of the comments, but I had some additional comments on the rest + some other things that I have noticed.
|
Well, it looks like the conversion to ISO strings was easier than expected! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
EventEntity.time | ||
>= datetime.strptime(range_start, "%d/%m/%Y, %H:%M:%S"), | ||
EventEntity.time <= datetime.strptime(range_end, "%d/%m/%Y, %H:%M:%S"), | ||
EventEntity.time >= datetime.fromisoformat(range_start), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Standards for the win!
This major pull request refactors the entire events feature to the new standards of Angular v17. This pull request also rewrites most of the functionality to be more concise and readable, following the best conventions for future students in COMP 423 and 393, using everything we have learned so far.
This refactor also fully utilizes the new frontend pagination abstraction created in #456.
Once again, I am adding @jadekeegan and @atoneyd to this PR as reviewers, since this was the feature we worked on the most! @atoneyd, you can also see the first glimpses of the new age of pagination in our site - thank you for helping to get the first version working, which was the stepping stone that allowed the abstraction to be made!!✨ @jadekeegan, this PR also iterates on event registrations.
Managing State of Paginated Data
Intro
This refactor was the first one that featured pagination as the main form of data retrieval, marking the first usage of the
Paginator<T>
abstraction introduced in #456. This is a refactor using Angular v17. We can combine the signals functionality with the abstracted paginator to make our code much more readable and performant.The Event Service
To use our abstraction, the new event service creates an encapsulated event paginator object, as shown below:
csxl.unc.edu/frontend/src/app/event/event.service.ts
Lines 34 to 36 in fcf9e16
The
TimeRangePaginator
is an implementation of thePaginatorAbstraction
abstract class which specifically works with paginated data on a time range, where a start and end time are used as parameters.Loading our new pages is extremely easy and is done in the
getEvents()
method, shown below:csxl.unc.edu/frontend/src/app/event/event.service.ts
Lines 47 to 49 in fcf9e16
This method takes in an optional
params
field for the pagination's parameters. In the case that this is not supplied, a default time range parameter constant is used. This saves us a lot of repeated code elsewhere in the feature.As mentioned in #456,
.loadPage()
takes in both params and an operator function in the case that the model returned by the API is not of the same type expected. Since our API returnsEventJson
, we can pass inparseEventJson
into this parameter, resulting in paginated data of typeEvent
.Notice that, unlike the organization refactor ( #455 ), the service does not expose any signals. This is because, unlike with organizations, events does not have a single collection of events shared by every component. We only want to load the events we need for a page (the entire point of pagination), so sharing this data does not really make sense here. Not to mention, pagination parameters are component specific and based on query parameters. So, this management is left up to the components.
The rest of the service methods follow normal conventions and return observables.
The Event Page Component
Pagination truly shines in the event page component, which got the heaviest refactor - this component shrunk nearly in half after the refactor.
First, our component has a reactive
page
signal that stores the current paginated event data. This signal will update every time a new page is loaded to store the new page, allowing our page to dynamically update.In addition, we previously needed to use a function to group event data by date. This functionality was moved into a new pipe called
GroupEventsPipe
. Now, the component also adds a computed signal which, when called (and the page updates), the signal uses the new pipe to expose grouped event data. This is done as shown below:csxl.unc.edu/frontend/src/app/event/event-page/event-page.component.ts
Lines 51 to 55 in fcf9e16
The best part is that we can make our specific pagination parameters as signals. There are three parameters - one for the start date, end date, and any filter query. This is shown below:
csxl.unc.edu/frontend/src/app/event/event-page/event-page.component.ts
Lines 56 to 62 in fcf9e16
This functionality is great paired with an effect. The new component has an effect such that, when any of the pagination signals are updated, the page automatically refreshes with new data and the query parameters in the URL is updated:
csxl.unc.edu/frontend/src/app/event/event-page/event-page.component.ts
Lines 81 to 100 in fcf9e16
This is great, since our page now updates whenever a query parameter is changed - which is done when the search bar is edited, or when a user presses on the arrows to switch between pages.
The Event Detail Component
Nothing major changed in the event detail component's refactor, although some code has been cleaned up. This component still uses the resolver to load a single event. This is required for the
titleResolver
to run properly.The Event Editor Component
The event editor has also been refactored to make the code cleaner and more concise.
In this refactor, the authentication functionality was moved to a new guard. Since this guard uses two different conditions to ensure that the user can access the page, I had to use the
combineLatest
RxJS method:csxl.unc.edu/frontend/src/app/event/event-editor/event-editor.guard.ts
Lines 45 to 50 in fcf9e16
Notice though that this guard has to call the
getEvent()
function from the service - which repeats functionality in the event resolver. This means that when we open the editor, two GET API calls are performed. This is really unfortunate, and I tried many different approaches, none of them work.The problem is that resolvers run before
canActivate
guards in Angular, so there was no way to use the resolver'sevent
data in the guard. The only other idea I saw online was to perform a reroute in the resolver if the user is not an organizer, however I felt that this overloaded the resolver with functionality that it should not have - thoughts @KrisJordan?Widgets
All widget HTML code was also updated to the new conventions!
Other Changes
Removal of the Event Admin Page and Gears
Throughout the refactoring process, I found the event admin component and related functionality merged in #402 needs to be reworked - so, I removed the admin page entirely. We will revisit this page once we get to the Angular Material 3 upgrade. No functionality is lost to the user with this removed, and the code becomes much cleaner.
Reversion to a Single Column Format
I have temporarily disabled the dual-column arrangement on the events page, since you recommended this @KrisJordan (@jadekeegan is very sad 😭)
I mainly disabled it because the code became much more concise - even if we add it back, I think the way it was implemented needs reworking.
File Structure Changes
There are not extremely major change to the file structure in the events page, however a new
/pipe
directory has been added to follow the conventions established in #455.Future Considerations
titleResolver
and the child route added to get it configured.