-
-
Notifications
You must be signed in to change notification settings - Fork 174
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
Feature: Use Mantine ScrollArea #485
base: v2
Are you sure you want to change the base?
Conversation
…her/mantine-react-table into feature/mantine-scrollArea
@studylog-sbasher is attempting to deploy a commit to the Kevin Vandy OSS Team on Vercel. A member of the Team first needs to authorize it. |
bottomToolbarRef.current?.offsetHeight, | ||
]); | ||
|
||
useLayoutEffect(() => { |
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.
this will need to be isomorphic
@@ -69,10 +71,16 @@ export const useMRT_ColumnVirtualizer = < | |||
[draggingColumn?.id], | |||
); | |||
|
|||
useLayoutEffect(() => { | |||
if (withScrollArea && !table.refs.scrollAreaViewportRef.current) { |
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.
This feels like the wrong place for this code. Probably should put it in the ref assignment as a useCallback
@@ -60,7 +71,7 @@ export const useMRT_RowVirtualizer = < | |||
overscan: 4, | |||
rangeExtractor: useCallback( | |||
(range: Range) => { | |||
return extraIndexRangeExtractor(range, draggingRow?.index ?? 0); | |||
return extraIndexRangeExtractor(range, draggingRow?.index); |
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.
this fixes a bug I recently fixed in MUI MRT too
} | ||
: {}; | ||
|
||
const ScrollWrapper = withScrollArea ? Table.ScrollContainer : Fragment; |
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.
Should the ScrollWrapper just go in the TableContainer component and replace the default Box if enabled?
const scrollContainerProps = withScrollArea | ||
? { | ||
className: classes.scrollContainer, | ||
h: '100%', |
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.
There's a bug where the scroll area will not grow in size when the table height grows, such as when a table enters full-screen
Also, since we're still in beta, we could just totally replace the current scrolling setup and just go with the stock mantine one. It would be a minor (mostly non-breaking) change before a stable release. It's a bit complicated with the two different ways. |
Hey @sbasher314 just checking if you're still available to continue working in this PR? |
Hey yeah, started a new job so things have been pretty busy but I should have capacity to work on this shortly. I should be able to make the requested changes |
Adds Mantine
ScrollArea
component as an optional wrapper for the table body; Supports virtualization as well as standard / non-virtualized tables.Usage:
withScrollArea
to the table props to enable this feature -- disabled by default so as to not break back-compatibilitymantineScrollAreaProps
for further customization if neededI've fleshed out a few storybook examples as well as a few examples on the docs (specifically virtualization / infinite loading). Let me know if I missed anything or if there's any room for improvement!
Address the following discussion / issue : #3