-
Notifications
You must be signed in to change notification settings - Fork 937
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
Conversation
return tlRender(<ThemeProviders>{component}</ThemeProviders>); | ||
export const renderComponent = (component: React.ReactElement) => { | ||
return tlRender(component, { | ||
wrapper: ({ children }) => <ThemeProviders>{children}</ThemeProviders>, |
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.
Had to make this change because not using the wrapper causes manual re-renders to break during testing
Gonna have to make a couple of changes to the stories, but most of them cleared |
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! 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(); |
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.
I think in the jest setup file we already clear all the mocks after each test.
); | ||
|
||
await assertNoScroll(mockScroll); | ||
unmount(); |
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.
Why do we have to explicitly unmount?
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.
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 |
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.
Do we know the reason?
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.
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" |
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.
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.
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.
Yeah, I agree. I can rename it
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.
Code looks good but I have mixed feelings. For me, it looks we are adding a lot of complexity to handle scrolling.
@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. |
@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 |
@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 |
Closes #10589 (in combination with #10803).
Changes made
PaginationContainer
component that:usePaginatedQuery
usePaginatedQuery
to make generating table labels easierrenderComponent
test utility so that it can safely re-render without any risks of throwing out the previous render stateNotes