feat: add pagination support in templates page #1131
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
✅ Deploy Preview for docs-novu canceled.
|
There was a problem hiding this comment.
This is amazing @jainpawan21!!! We are so humbled to have you as a contributor.
My main note here would be to avoid using the usePagination flag on the API and enable pagination by default while allowing the user passing LIMIT with some limits. Let me know what you think
| return this.getNotificationTemplatesUsecase.execute( | ||
| GetNotificationTemplatesCommand.create({ | ||
| organizationId: user.organizationId, | ||
| userId: user._id, | ||
| environmentId: user.environmentId, | ||
| page: query.page ? Number(query.page) : 0, | ||
| usePagination: query.usePagination === 0 ? false : true, |
There was a problem hiding this comment.
I think that if we are going this direction we can just enable pagination by default, and instead of passing usePagination, we can allow the user to pass the limit property which by default would be for example 15 entries~ and maximum value allowed would be 100 for some API usecases. Wdyt?
| async execute(command: GetNotificationTemplatesCommand): Promise<NotificationTemplateEntity[]> { | ||
| const list = await this.notificationTemplateRepository.getList(command.organizationId, command.environmentId); | ||
| async execute(command: GetNotificationTemplatesCommand): Promise<NotificationTemplatesResponseDto> { | ||
| const LIMIT = 10; |
There was a problem hiding this comment.
This limit could be passed from the API User with a default of 10 and maximum at 100, you can validate using class-validators on the API usecase. if you could also verify in other places of the application if we use this usecase aswell in case we do some "client-side" search on the server
| @@ -21,7 +21,7 @@ interface IFiltersForm { | |||
| } | |||
|
|
|||
| export function ActivitiesPage() { | |||
| const { templates, loading: loadingTemplates } = useTemplates(); | |||
| const { templates, loading: loadingTemplates } = useTemplates(0, false); | |||
There was a problem hiding this comment.
Here instead of passing the usePagiatnion, we can just pass the maximum value 100, and in the future just do a server side text search to fix the large scale issues here.
| getNotificationTemplates( | ||
| @UserSession() user: IJwtPayload, | ||
| @Query() query: NotificationTemplatesRequestDto | ||
| ): Promise<NotificationTemplatesResponseDto> { |
There was a problem hiding this comment.
This is amazing, could we also add a e2e API tests for the behaviour of pagination ?
Thanks for your kind words Dima 🙏🏻, |
|
@jainpawan21 for now we can just make sure to load the maximum page amount of 100~ templates on initial loads and than we can load all the subsequent pages if they are exist. I think it's a bit of an edge case for now so shouldn't be a problem for most of our users. |
|
@scopsy is attempting to deploy a commit to the Novu Team on Vercel. A member of the Team first needs to authorize it. |
…in-templates-page' into feature/pagination-in-templates-page
feature: add pagination support in templates page.
Long list of notifications. User have to scroll a lot
Screen.Recording.2022-08-26.at.10.51.31.PM.mov