fix(notification-center,widget): infinite scroll issue #5371
Conversation
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for novu-design ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for novu-design ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -171,11 +171,12 @@ describe('Notifications List', function () { | |||
| cy.getByTestId('unseen-count-label').should('contain', '99+'); | |||
| }); | |||
|
|
|||
| it('pagination check', async function () { | |||
There was a problem hiding this comment.
because of async this test was always passing, no matter of the body results
| cy.wait('@getNotificationsFirstPage'); | ||
| cy.task('createNotifications', { | ||
| organizationId: this.session.organization._id, | ||
| enumerate: true, | ||
| ordered: true, |
There was a problem hiding this comment.
this test verifies notifications in the order, so I've added the option to create notifications ordered (trigger with a small delay after)
| @@ -206,5 +207,5 @@ describe('Notifications List', function () { | |||
| }); | |||
|
|
|||
| function scrollToBottom() { | |||
| cy.getByTestId('notifications-scroll-area').get('.infinite-scroll-component').scrollTo('bottom'); | |||
| cy.getByTestId('notifications-scroll-area').scrollTo('bottom', { ensureScrollable: true }); | |||
There was a problem hiding this comment.
ensureScrollable ensures that the target element is scrollable element, otherwise the test will fail
There was a problem hiding this comment.
✏ suggestion: This is helpful context in the code
| @@ -47,4 +47,5 @@ export function NotificationsList({ | |||
|
|
|||
| const notificationsListCss = css` | |||
| height: 400px; | |||
| overflow-y: auto; | |||
There was a problem hiding this comment.
the fix... the InfiniteScroll component has a prop scrollableTarget="notifications-list" which points to the parent element and it should be a scrollable element otherwise the infinite scroll won't work
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -206,5 +207,5 @@ describe('Notifications List', function () { | |||
| }); | |||
|
|
|||
| function scrollToBottom() { | |||
| cy.getByTestId('notifications-scroll-area').get('.infinite-scroll-component').scrollTo('bottom'); | |||
| cy.getByTestId('notifications-scroll-area').scrollTo('bottom', { ensureScrollable: true }); | |||
There was a problem hiding this comment.
✏ suggestion: This is helpful context in the code
fix(notification-center,widget): infinite scroll issue
|
Nicely done @LetItRock for the quick fix. We can take the fix a step further and hide the scrollbar all together while scrolling giving a more polished feeling to the UI especially in non OS devices |
fix(notification-center,widget): infinite scroll issue
What change does this PR introduce?
Fixes the issue introduced in: #5019, with not scrollable parent component.
Fixes the Widget test for the infinite scroll which was always passing, and makes sure that the scroll event is triggered on the scrollable element.
Why was this change needed?
Other information (Screenshots)
Screen.Recording.2024-04-05.at.22.06.21.mov
Screen.Recording.2024-04-05.at.22.28.59.mov