Skip to content

fix: create centralized PaginationContainer component #10967

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

Merged
merged 21 commits into from
Dec 2, 2023
Merged

Conversation

Parkreiner
Copy link
Member

@Parkreiner Parkreiner commented Nov 30, 2023

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

return tlRender(<ThemeProviders>{component}</ThemeProviders>);
export const renderComponent = (component: React.ReactElement) => {
return tlRender(component, {
wrapper: ({ children }) => <ThemeProviders>{children}</ThemeProviders>,
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@Parkreiner Parkreiner changed the title fix: create centralized Pagination component fix: create centralized PaginationContainer component Dec 1, 2023
@Parkreiner Parkreiner marked this pull request as ready for review December 1, 2023 13:12
@Parkreiner Parkreiner requested review from BrunoQuaresma, a team and Kira-Pilot and removed request for a team December 1, 2023 13:12
@Parkreiner
Copy link
Member Author

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

Copy link
Member

@Kira-Pilot Kira-Pilot left a comment

Choose a reason for hiding this comment

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

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.

});

afterAll(() => {
jest.clearAllMocks();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

);

await assertNoScroll(mockScroll);
unmount();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we have to explicitly unmount?

Copy link
Member Author

@Parkreiner Parkreiner Dec 2, 2023

Choose a reason for hiding this comment

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

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

});

// Would've liked to consolidate these effects into a single useLayoutEffect
// call, but they kept messing each other up when grouped together
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we know the reason?

Copy link
Member Author

@Parkreiner Parkreiner Dec 2, 2023

Choose a reason for hiding this comment

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

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

currentPage={page}
<PaginationContainer
paginationResult={usersQuery}
paginationUnitLabel="users"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@Parkreiner Parkreiner Dec 2, 2023

Choose a reason for hiding this comment

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

Yeah, I agree. I can rename it

Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a comment

Choose a reason for hiding this comment

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

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

@BrunoQuaresma
Copy link
Collaborator

@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
Copy link
Member Author

Parkreiner commented Dec 2, 2023

@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
Copy link
Member Author

@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 Parkreiner merged commit 28eca2e into main Dec 2, 2023
@Parkreiner Parkreiner deleted the mes/pagination-3 branch December 2, 2023 22:38
@github-actions github-actions bot locked and limited conversation to collaborators Dec 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding safety nets and better performance to paginated tables
3 participants