Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[DataGrid] Support scrolling over column headers under experimental flag #8343

Closed
wants to merge 4 commits into from
Closed

[DataGrid] Support scrolling over column headers under experimental flag #8343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5aa46b
support scrolling over column headers under experimental flag
cherniavskii Mar 22, 2023
f87e15c
Merge branch 'master' into scroll-over-column-headers
cherniavskii Apr 27, 2023
7bd564d
do not call preventDefault if the scroll position hasn't changed
cherniavskii Apr 27, 2023
bdf9c25
list to wheel even on columnHeaders container instead of inner element
cherniavskii Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/pages/x/api/data-grid/data-grid-premium.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@
"experimentalFeatures": {
"type": {
"name": "shape",
"description": "{ columnGrouping?: bool, lazyLoading?: bool, warnIfFocusStateIsNotSynced?: bool }"
"description": "{ columnGrouping?: bool, lazyLoading?: bool, scrollOverColumnHeaders?: bool, warnIfFocusStateIsNotSynced?: bool }"
}
},
"filterMode": {
Expand Down
2 changes: 1 addition & 1 deletion docs/pages/x/api/data-grid/data-grid-pro.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
"experimentalFeatures": {
"type": {
"name": "shape",
"description": "{ columnGrouping?: bool, lazyLoading?: bool, warnIfFocusStateIsNotSynced?: bool }"
"description": "{ columnGrouping?: bool, lazyLoading?: bool, scrollOverColumnHeaders?: bool, warnIfFocusStateIsNotSynced?: bool }"
}
},
"filterMode": {
Expand Down
2 changes: 1 addition & 1 deletion docs/pages/x/api/data-grid/data-grid.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
"experimentalFeatures": {
"type": {
"name": "shape",
"description": "{ columnGrouping?: bool, warnIfFocusStateIsNotSynced?: bool }"
"description": "{ columnGrouping?: bool, scrollOverColumnHeaders?: bool, warnIfFocusStateIsNotSynced?: bool }"
}
},
"filterMode": {
Expand Down
1 change: 1 addition & 0 deletions packages/grid/x-data-grid-premium/src/DataGridPremium/DataGridPremium.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ DataGridPremiumRaw.propTypes = {
experimentalFeatures: PropTypes.shape({
columnGrouping: PropTypes.bool,
lazyLoading: PropTypes.bool,
scrollOverColumnHeaders: PropTypes.bool,
warnIfFocusStateIsNotSynced: PropTypes.bool,
}),
/**
Expand Down
1 change: 1 addition & 0 deletions packages/grid/x-data-grid-pro/src/DataGridPro/DataGridPro.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ DataGridProRaw.propTypes = {
experimentalFeatures: PropTypes.shape({
columnGrouping: PropTypes.bool,
lazyLoading: PropTypes.bool,
scrollOverColumnHeaders: PropTypes.bool,
warnIfFocusStateIsNotSynced: PropTypes.bool,
}),
/**
Expand Down
1 change: 1 addition & 0 deletions packages/grid/x-data-grid/src/DataGrid/DataGrid.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ DataGridRaw.propTypes = {
*/
experimentalFeatures: PropTypes.shape({
columnGrouping: PropTypes.bool,
scrollOverColumnHeaders: PropTypes.bool,
warnIfFocusStateIsNotSynced: PropTypes.bool,
}),
/**
Expand Down
32 changes: 32 additions & 0 deletions packages/grid/x-data-grid/src/hooks/features/columnHeaders/useGridColumnHeaders.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,38 @@ export const useGridColumnHeaders = (props: UseGridColumnHeadersProps) => {
apiRef.current.columnHeadersContainerElementRef!.current!.scrollLeft = 0;
}, [apiRef]);

const scrollOverColumnHeaders = rootProps.experimentalFeatures?.scrollOverColumnHeaders;
React.useEffect(() => {
if (!scrollOverColumnHeaders) {
// TODO: uncomment before merging
// return undefined;
}
const rootElement = apiRef.current.columnHeadersContainerElementRef?.current;
if (!rootElement) {
return undefined;
}

const callback = (event: WheelEvent) => {
const virtualScroller = apiRef.current.virtualScrollerRef?.current;
if (!virtualScroller) {
return;
}

const previousScrollLeft = virtualScroller.scrollLeft;
virtualScroller.scrollLeft += event.deltaX;

// If the scroll position hasn't changed - it's the end of the scroll
if (virtualScroller.scrollLeft !== previousScrollLeft) {
event.preventDefault();
Copy link
Member

Choose a reason for hiding this comment

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

If the preventDefault is to prevent going to the previous page if the user scrolls left (when already on the leftmost position), I'll vote not to add this, as this will negate the default browser behavior/user expectation.

For example when I try to scroll to the left when already on the leftmost column in the virtual scroller it goes to the previous page, the same is the case somewhere on the page (outside the grid), so my expectation would be the same for the header area too.

grid-swipe-left

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is that deltaX may not be equal to 0, even if the element is not scrolling. This is causing preventDefault to be called unnecessarily every time. To solve this problem, we can replace deltaX with prevScrollLeft.current, which holds the previous scroll position. This ensures that preventDefault is only called when the element is actually scrolling.

if (prevScrollLeft.current !== 0) 
        event.preventDefault();
bandicam.2023-03-24.15-55-35-983.mp4

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to call preventDefault conditionally, and it kind of works, but sometimes calling preventDefault doesn't prevent the default browser action:

Screen.Recording.2023-04-28.at.14.16.05.mov

It's very unreliable and I see two options here:

  1. Roll back to always call preventDefault
  2. Close this PR and potentially look for a better solution to the problem

I would prefer option 1, because it's better than nothing, but I understand that it's not perfect :)
What do you think?
@MBilalShafi
@joserodolfofreitas

}
};
rootElement.addEventListener('wheel', callback);

return () => {
rootElement.removeEventListener('wheel', callback);
};
}, [innerRef, apiRef, scrollOverColumnHeaders]);

// memoize `getFirstColumnIndexToRender`, since it's called on scroll
const getFirstColumnIndexToRenderRef = React.useRef<typeof getFirstColumnIndexToRender>(
defaultMemoize(getFirstColumnIndexToRender, {
Expand Down
4 changes: 4 additions & 0 deletions packages/grid/x-data-grid/src/models/props/DataGridProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ export interface GridExperimentalFeatures {
* Only works if NODE_ENV=test.
*/
warnIfFocusStateIsNotSynced: boolean;
/**
* If `true`, the users will be able to scroll the grid horizontally when the cursor is over the column headers.
*/
scrollOverColumnHeaders: boolean;
}

/**
Expand Down