[DataGrid] Add scroll shadows and fix scrollbar overlap #16476
+309
−191
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cherniavskii marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Looks like scroll shadows do not consider RTL and don't work properly There was a problem hiding this comment. Good spot. Updated 392d9a0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| import * as React from 'react'; | ||
| import { styled } from '@mui/system'; | ||
| import { useRtl } from '@mui/system/RtlProvider'; | ||
| import { | ||
| gridDimensionsSelector, | ||
| gridPinnedColumnsSelector, | ||
| useGridEvent, | ||
| useGridSelector, | ||
| } from '../hooks'; | ||
| import { gridPinnedRowsSelector } from '../hooks/features/rows/gridRowsSelector'; | ||
| import { useGridRootProps } from '../hooks/utils/useGridRootProps'; | ||
| import { DataGridProcessedProps } from '../models/props/DataGridProps'; | ||
| import { GridEventListener } from '../models/events'; | ||
| import { vars } from '../constants/cssVariables'; | ||
| import { useGridPrivateApiContext } from '../hooks/utils/useGridPrivateApiContext'; | ||
|
|
||
| interface GridScrollShadowsProps { | ||
| position: 'vertical' | 'horizontal'; | ||
| } | ||
|
|
||
| type OwnerState = Pick<DataGridProcessedProps, 'classes'> & { | ||
| position: 'vertical' | 'horizontal'; | ||
| }; | ||
|
|
||
| const ScrollShadow = styled('div')<{ ownerState: OwnerState }>(({ theme }) => ({ | ||
|
There was a problem hiding this comment. The shadows look quite dim in dark mode: 
I can't tell that there are pinned rows and columns just looking at the screenshot 😅 It looks great in light mode for comparison: 
There was a problem hiding this comment. Shadows on dark mode are hard 🫠 We need to have a lighter background on the grid for shadows to be effective. Investigated a little and found that on the Material UI Table component, it has the paper overlay making it a dark grey as opposed to the black we currently have: 
I've updated the background color on dark mode for the data grid to match and refined the shadow size/opacity: 
Feels like an improvement — WDYT? |
||
| position: 'absolute', | ||
| inset: 0, | ||
| pointerEvents: 'none', | ||
| transition: vars.transition(['box-shadow'], { duration: vars.transitions.duration.short }), | ||
| '--length': theme.palette.mode === 'dark' ? '8px' : '6px', | ||
| '--length-inverse': 'calc(var(--length) * -1)', | ||
| '--opacity': theme.palette.mode === 'dark' ? '0.7' : '0.18', | ||
| '--blur': 'var(--length)', | ||
| '--spread': 'calc(var(--length) * -1)', | ||
| '--color-start': 'rgba(0, 0, 0, calc(var(--hasScrollStart) * var(--opacity)))', | ||
| '--color-end': 'rgba(0, 0, 0, calc(var(--hasScrollEnd) * var(--opacity)))', | ||
| variants: [ | ||
| { | ||
| props: { position: 'vertical' }, | ||
| style: { | ||
| top: 'var(--DataGrid-topContainerHeight)', | ||
| bottom: | ||
| 'calc(var(--DataGrid-bottomContainerHeight) + var(--DataGrid-hasScrollX) * var(--DataGrid-scrollbarSize))', | ||
| boxShadow: | ||
| 'inset 0 var(--length) var(--blur) var(--spread) var(--color-start), inset 0 var(--length-inverse) var(--blur) var(--spread) var(--color-end)', | ||
| }, | ||
| }, | ||
| { | ||
| props: { position: 'horizontal' }, | ||
| style: { | ||
| left: 'var(--DataGrid-leftPinnedWidth)', | ||
| right: | ||
| 'calc(var(--DataGrid-rightPinnedWidth) + var(--DataGrid-hasScrollY) * var(--DataGrid-scrollbarSize))', | ||
| boxShadow: | ||
| 'inset var(--length) 0 var(--blur) var(--spread) var(--color-start), inset var(--length-inverse) 0 var(--blur) var(--spread) var(--color-end)', | ||
| }, | ||
| }, | ||
| ], | ||
| })); | ||
|
|
||
| function GridScrollShadows(props: GridScrollShadowsProps) { | ||
| const { position } = props; | ||
| const rootProps = useGridRootProps(); | ||
| const ownerState = { classes: rootProps.classes, position }; | ||
| const ref = React.useRef<HTMLDivElement>(null); | ||
| const apiRef = useGridPrivateApiContext(); | ||
| const dimensions = useGridSelector(apiRef, gridDimensionsSelector); | ||
| const pinnedRows = useGridSelector(apiRef, gridPinnedRowsSelector); | ||
| const pinnedColumns = useGridSelector(apiRef, gridPinnedColumnsSelector); | ||
| const initialScrollable = | ||
| position === 'vertical' | ||
| ? dimensions.hasScrollY && pinnedRows?.bottom?.length > 0 | ||
| : dimensions.hasScrollX && | ||
| pinnedColumns?.right?.length !== undefined && | ||
| pinnedColumns?.right?.length > 0; | ||
| const isRtl = useRtl(); | ||
|
|
||
| const updateScrollShadowVisibility = (scrollPosition: number) => { | ||
| if (!ref.current) { | ||
| return; | ||
| } | ||
| // Math.abs to convert negative scroll position (RTL) to positive | ||
| const scroll = Math.abs(Math.round(scrollPosition)); | ||
| const maxScroll = Math.round( | ||
| dimensions.contentSize[position === 'vertical' ? 'height' : 'width'] - | ||
| dimensions.viewportInnerSize[position === 'vertical' ? 'height' : 'width'], | ||
| ); | ||
| const hasPinnedStart = | ||
| position === 'vertical' | ||
| ? pinnedRows?.top?.length > 0 | ||
| : pinnedColumns?.left?.length !== undefined && pinnedColumns?.left?.length > 0; | ||
| const hasPinnedEnd = | ||
| position === 'vertical' | ||
| ? pinnedRows?.bottom?.length > 0 | ||
| : pinnedColumns?.right?.length !== undefined && pinnedColumns?.right?.length > 0; | ||
| const scrollIsNotAtStart = isRtl ? scroll < maxScroll : scroll > 0; | ||
| const scrollIsNotAtEnd = isRtl ? scroll > 0 : scroll < maxScroll; | ||
| ref.current.style.setProperty( | ||
| '--hasScrollStart', | ||
| hasPinnedStart && scrollIsNotAtStart ? '1' : '0', | ||
| ); | ||
| ref.current.style.setProperty('--hasScrollEnd', hasPinnedEnd && scrollIsNotAtEnd ? '1' : '0'); | ||
| }; | ||
|
|
||
| const handleScrolling: GridEventListener<'scrollPositionChange'> = (scrollParams) => { | ||
| updateScrollShadowVisibility(scrollParams[position === 'vertical' ? 'top' : 'left']); | ||
| }; | ||
|
|
||
| const handleColumnResizeStop: GridEventListener<'columnResizeStop'> = () => { | ||
| if (position === 'horizontal') { | ||
| updateScrollShadowVisibility(apiRef.current.virtualScrollerRef?.current?.scrollLeft || 0); | ||
| } | ||
| }; | ||
|
|
||
| useGridEvent(apiRef, 'scrollPositionChange', handleScrolling); | ||
| useGridEvent(apiRef, 'columnResizeStop', handleColumnResizeStop); | ||
|
|
||
| return ( | ||
| <ScrollShadow | ||
| ownerState={ownerState} | ||
| ref={ref} | ||
| style={ | ||
| { | ||
| '--hasScrollStart': 0, | ||
| '--hasScrollEnd': initialScrollable ? '1' : '0', | ||
| } as React.CSSProperties | ||
| } | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| export { GridScrollShadows }; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
So now the empty cell above fills the space
ScrollbarFillerwas filling, correct?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.
My understanding is that scrollbar fillers on rows is currently only necessary to cover the area where the scrollbar is not present on pinned rows:
Now that the scrollbar covers this area, I don't think it is necessary 🤔