Upgrade and virtualize table component #8157
Conversation
There was a problem hiding this comment.
I noticed that there's some extra double scrolling going on now… Ideally it would appear like a normal settings page that just scrolls forever. If that's too hard with the virtualization then the main page scrolling could be fixed, but either way there shouldn't be multiple scroll areas :)
Oh, I think the table height needs adjusting. I intentionally fixed the page height so that only the table area is scrollable. That way, the filters at the top are always accessible.
Both should be doable. |
In a perfect world it would be a full page scroll as that then triggers the sticky header instead of having the top 30% of the screen be used up by the header. Otherwise this is great though, thanks |
Couldn't agree more! I tried that initially but it was very tricky to get it right for table and absolute positioned elements (quite a few quirks for table in CSS spec). An alternative we can try is to replace tables with divs and aria roles. That might help a lot in navigating these quirks. Is that something we can explore? |
|
Happy to explore that if the table is the limiting factor |
|
I just pushed a commit that implements the full page scroll, please test and let me know if something needs to be changed 😄 |
app/components/FilterOptions.tsx
Outdated
| }: Props) => { | ||
| const { t } = useTranslation(); | ||
| const searchInputRef = React.useRef<HTMLInputElement>(null); | ||
| const listRef = React.useRef<HTMLDivElement | null>(null); | ||
| const menu = useMenuState({ | ||
| modal: true, | ||
| modal: modalMenu ?? true, |
There was a problem hiding this comment.
Quirks of reakit.. scroll position changes unexpectedly when opening the menu in modal mode
ariakit/ariakit#856 - Seems this issue is fixed in v2; all the more reason to move to Radix soon.
There was a problem hiding this comment.
Ok, I guess my only thought is can we just default this to false then and avoid the prop drilling? It's not used in many spots
There was a problem hiding this comment.
With modal true, the tab focus stays within the rendered menu - it may not be as important in this case I think.
Will change it 👍
app/components/Scene.tsx
Outdated
| @@ -21,6 +21,8 @@ type Props = { | |||
| wide?: boolean; | |||
| /** The content of the scene */ | |||
| children?: React.ReactNode; | |||
| /** Whether to shrink padding */ | |||
| shrink?: boolean; | |||
There was a problem hiding this comment.
I'm not really clear on how this differs from the wide option – aka when would we use one without the other? Would prefer to avoid shrink if wide can suffice.
There was a problem hiding this comment.
Yeah, this is not needed now..
Earlier, with the fixed height table, this was needed to reduce the bottom padding (and hence the body scroll).
app/components/FilterOptions.tsx
Outdated
| }: Props) => { | ||
| const { t } = useTranslation(); | ||
| const searchInputRef = React.useRef<HTMLInputElement>(null); | ||
| const listRef = React.useRef<HTMLDivElement | null>(null); | ||
| const menu = useMenuState({ | ||
| modal: true, | ||
| modal: modalMenu ?? true, |
There was a problem hiding this comment.
Ok, I guess my only thought is can we just default this to false then and avoid the prop drilling? It's not used in many spots
|
|
||
| const appName = env.APP_NAME; | ||
| React.useEffect(() => { | ||
| const timeout = setTimeout(() => updateParams("query", query), 250); |
There was a problem hiding this comment.
Why the timeout? Probably fine but worth a comment
There was a problem hiding this comment.
It's more of a "hacky" debounce for the filter typed in the search box (earlier, we were invoking the API for each keypress).
Oop, just spotted the sticky filters are infront of dialogs |
This PR virtualizes the
Tablecomponent to fetch and show more data on scroll instead of prev/back buttons. Along the way,react-tablelibrary is upgraded to the latest version (tanstack/react-table).For virtualization,
tanstack/react-virtualis used instead of the existingreact-windowlibrary.tanstack/react-virtualAPIs are more modern and more importantly, it usestransformto show/hide virtualized items (react-windowusestop- we should replace its existing usages).