fix: create centralized PaginationContainer component #10967

Parkreiner · 2023-11-30T23:57:06Z

Closes #10589 (in combination with #10803).

Changes made

Creates general-purpose PaginationContainer component that:
- Centralizes a lot of pagination view concerns that were split across several different components throughout the codebase
- Adds the component to the Users and Audits tables
- Is set up to for easy interfacing with usePaginatedQuery
- Handles auto-scrolling the user to the top of a table when the page changes (doing it in the least aggressive way possible, with tests in place to help verify that scrolls only happen when they won't be obtrusive to the user)
- Has updated logic for generating labels for the tops of each table (numbers are now always correct)
Updates the return type for usePaginatedQuery to make generating table labels easier
Updates the renderComponent test utility so that it can safely re-render without any risks of throwing out the previous render state

Notes

Pasting this from the other PR: untangling the Workspaces table from its current implementation proved to be a little too much for this sprint; had to defer switching that table over to a later sprint
- Still tried to bring over as much of the new code as I could
There's a lot of disparate steps to the scrolling logic for PaginationContainer; it might help to look at the names of each of its test cases first just to get an idea for what it's trying to account for

…ation tests

Parkreiner · 2023-12-01T00:09:56Z

site/src/testHelpers/renderHelpers.tsx

-  return tlRender(<ThemeProviders>{component}</ThemeProviders>);
+export const renderComponent = (component: React.ReactElement) => {
+  return tlRender(component, {
+    wrapper: ({ children }) => <ThemeProviders>{children}</ThemeProviders>,


Had to make this change because not using the wrapper causes manual re-renders to break during testing

Parkreiner · 2023-12-01T15:29:10Z

Gonna have to make a couple of changes to the stories, but most of them cleared
Worst example was one of the stories displaying NaN values

Kira-Pilot

Nice! I pulled down and took a look - works really well.
One piece of non-blocking feedback that we could look at in another PR: I find it a little cumbersome to scroll through an entire page of records to access the pagination widget. I click, am scrolled up to the top, and then have to scroll down again to find the widget. I wonder if we could just move the widget to the top of the page or duplicate it in a simpler form.

BrunoQuaresma · 2023-12-01T15:48:32Z

site/src/components/PaginationWidget/PaginationContainer.test.tsx

+});
+
+afterAll(() => {
+  jest.clearAllMocks();


I think in the jest setup file we already clear all the mocks after each test.

BrunoQuaresma · 2023-12-01T15:48:58Z

site/src/components/PaginationWidget/PaginationContainer.test.tsx

+        );
+
+        await assertNoScroll(mockScroll);
+        unmount();


Why do we have to explicitly unmount?

Because the loop creates a new version of the UI per iteration. But if you don't unmount the previous one, the old UI from the last render sticks around and can cause some issues with test utilities/selectors

This happens automatically for most test cases, but it needs to be done manually here because multiple renders are happening before the test case gets a chance to be garbage-collected

BrunoQuaresma · 2023-12-01T15:50:47Z

site/src/components/PaginationWidget/PaginationContainer.tsx

+  });
+
+  // Would've liked to consolidate these effects into a single useLayoutEffect
+  // call, but they kept messing each other up when grouped together


Do we know the reason?

It's not because it's an unknown reason – it's more that there are two "sync cues" that the UI needs to account for, and trying to squeeze both of them into one layout effect just got a little unruly. It felt more manageable to set up one effect for the page changes, and one for the old data status, especially since they can change independently

I can rewrite the comment to make that more clear, though

BrunoQuaresma · 2023-12-01T15:54:02Z

site/src/pages/UsersPage/UsersPageView.tsx

-          currentPage={page}
+      <PaginationContainer
+        paginationResult={usersQuery}
+        paginationUnitLabel="users"


Nit: Since the component is already a Pagination thing I don't think we should prefix the props with pagination because the component name makes it clear enough IMO.

Yeah, I agree. I can rename it

BrunoQuaresma

Code looks good but I have mixed feelings. For me, it looks we are adding a lot of complexity to handle scrolling.

BrunoQuaresma · 2023-12-01T16:00:57Z

@Parkreiner if you are interested in design work, the improvement that @Kira-Pilot suggested is a good one. I was planning to refactor the pagination component design but I didn't have time yet. So let me know if you are interested or not, I know you have the Backstage integration stuff on your plate so np.

Parkreiner · 2023-12-02T00:31:52Z

@BrunoQuaresma Yeah, I agree with you about the complexity, and I wish there were a more straightforward way to side-step it (that's sort of why I put a lot of effort into the tests to at least help document what the logic is supposed to do)

But the pagination is one of the areas where turning on query caching caused the UI to get a little weird. Right now, the scrollbar inconsistently jumps around between page transitions (especially when jumping from/to pages that aren't fully filled out), so I felt like it'd be good to make sure the scrollbar is more consistent for all transitions

I think @Kira-Pilot 's idea about adding mini navigation sounds really good, but I don't know if I'll have time to do it for the rest of the year (I don't know how Backstage is going to slot into the next sprint yet). At least for the moment, though, I can flip the script – instead of consistently scrolling to the top of the container, it can consistently scroll to the bottom, so that the UI buttons stay within reach

Parkreiner · 2023-12-02T22:17:49Z

@BrunoQuaresma @Kira-Pilot I ended up removing the scroll functionality completely – the more I played with it, the more I realized that there were cases that weren't being handled properly, and I didn't want to increase the complexity more. I stashed the changes away here, so that it's easier to revisit things down the line

Also, scrolling to the bottom started breaking one of the AuditPage tests, which didn't seem great

Parkreiner added 5 commits November 30, 2023 23:18

chore: add Pagination component, add new test, and update other pagin…

5076aaf

…ation tests

fix: add back temp spacing for WorkspacesPageView

b3274e1

chore: update AuditPage to use Pagination

b053d8a

chore: update UsersPage to use Pagination

22cb113

refactor: move parts of Pagination into WorkspacesPageView

6615442

github-actions bot assigned Parkreiner Nov 30, 2023

Parkreiner commented Dec 1, 2023

View reviewed changes

Parkreiner added 4 commits December 1, 2023 00:30

fix: handle empty states for pagination labels better

0db7164

docs: rewrite comment for clarity

b9f07c3

refactor: rename components/properties for clarity

58c6973

fix: rename component files for clarity

90ef163

Parkreiner changed the title ~~fix: create centralized Pagination component~~ fix: create centralized PaginationContainer component Dec 1, 2023

Parkreiner added 3 commits December 1, 2023 13:04

chore: add story for PaginationContainer

a02babe

chore: rename story for clarity

a9f57f3

fix: handle undefined case better

fffc236

Parkreiner marked this pull request as ready for review December 1, 2023 13:12

Parkreiner requested review from BrunoQuaresma, a team and Kira-Pilot and removed request for a team December 1, 2023 13:12

Parkreiner added 2 commits December 1, 2023 15:03

fix: update imports for PaginationContainer mocks

5e94899

fix: update story values for clarity

7242ba2

Kira-Pilot approved these changes Dec 1, 2023

View reviewed changes

BrunoQuaresma reviewed Dec 1, 2023

View reviewed changes

BrunoQuaresma approved these changes Dec 1, 2023

View reviewed changes

Parkreiner added 4 commits December 2, 2023 01:29

fix: update scroll logic to go to the bottom instead of the top

f4fd2a3

fix: update mock setup for test

cac222f

fix: update stories

e47d55a

fix: remove scrolling functionality

80635a4

Parkreiner added 3 commits December 2, 2023 22:18

fix: remove deprecated property

77f15d0

refactor: rename prop

a9e0965

fix: remove debounce flake

21e2d73

Parkreiner merged commit 28eca2e into main Dec 2, 2023

Parkreiner deleted the mes/pagination-3 branch December 2, 2023 22:38

github-actions bot locked and limited conversation to collaborators Dec 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: create centralized PaginationContainer component #10967

fix: create centralized PaginationContainer component #10967

Uh oh!

Parkreiner commented Nov 30, 2023 •

edited

Loading

Uh oh!

Parkreiner Dec 1, 2023

Uh oh!

Parkreiner commented Dec 1, 2023

Uh oh!

Kira-Pilot left a comment

Uh oh!

BrunoQuaresma Dec 1, 2023

Uh oh!

BrunoQuaresma Dec 1, 2023

Uh oh!

Parkreiner Dec 2, 2023 •

edited

Loading

Uh oh!

BrunoQuaresma Dec 1, 2023

Uh oh!

Parkreiner Dec 2, 2023 •

edited

Loading

Uh oh!

BrunoQuaresma Dec 1, 2023

Uh oh!

Parkreiner Dec 2, 2023 •

edited

Loading

Uh oh!

BrunoQuaresma left a comment

Uh oh!

BrunoQuaresma commented Dec 1, 2023

Uh oh!

Parkreiner commented Dec 2, 2023 •

edited

Loading

Uh oh!

Parkreiner commented Dec 2, 2023

Uh oh!

Uh oh!

fix: create centralized PaginationContainer component #10967

fix: create centralized PaginationContainer component #10967

Uh oh!

Conversation

Parkreiner commented Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes made

Notes

Uh oh!

Parkreiner Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

Parkreiner commented Dec 1, 2023

Uh oh!

Kira-Pilot left a comment

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresma Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresma Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

Parkreiner Dec 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresma Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

Parkreiner Dec 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresma Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

Parkreiner Dec 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresma left a comment

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresma commented Dec 1, 2023

Uh oh!

Parkreiner commented Dec 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Parkreiner commented Dec 2, 2023

Uh oh!

Uh oh!

Parkreiner commented Nov 30, 2023 •

edited

Loading

Parkreiner Dec 2, 2023 •

edited

Loading

Parkreiner Dec 2, 2023 •

edited

Loading

Parkreiner Dec 2, 2023 •

edited

Loading

Parkreiner commented Dec 2, 2023 •

edited

Loading