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.

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

[DataGridPro] Fix missing border in right-pinned columns #4197

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
efd8aed
fix showCellRightBorder with columns pinned to the right
cherniavskii Mar 15, 2022
adae7ba
extract getBorderColor to styleUtils
cherniavskii Mar 15, 2022
7d9b595
add global theme control to storybook
cherniavskii Mar 15, 2022
5829054
add story
cherniavskii Mar 15, 2022
b9322f9
add unit test
cherniavskii Mar 15, 2022
744ffe3
empty commit to trigger codesandbox ci
cherniavskii Mar 15, 2022
73c8eaa
Merge branch 'master' into showCellRightBorder-right-pinned-columns
cherniavskii Mar 16, 2022
c5ba561
Merge branch 'master' into showCellRightBorder-right-pinned-columns
cherniavskii Mar 22, 2022
f0c1a47
Merge branch 'master' into showCellRightBorder-right-pinned-columns
cherniavskii Mar 22, 2022
e8d4e08
make `withBorder` only set border color
cherniavskii Mar 22, 2022
4a80b54
update api docs
cherniavskii Mar 22, 2022
ef756e4
Merge branch 'master' into showCellRightBorder-right-pinned-columns
cherniavskii Apr 4, 2022
2794e4f
Merge branch 'master' into showCellRightBorder-right-pinned-columns
cherniavskii Apr 4, 2022
c4aebba
split withRightBorder class into 2 separate classes
cherniavskii Apr 4, 2022
bed8f3b
build api docs
cherniavskii Apr 4, 2022
e3a1f3d
Merge branch 'master' into showCellRightBorder-right-pinned-columns
cherniavskii Apr 7, 2022
65a56cb
Merge branch 'next' into showCellRightBorder-right-pinned-columns
cherniavskii Dec 5, 2022
9677dab
eslint
cherniavskii Dec 5, 2022
ccf5cdb
add border to the right-pinned columns when showCellRightBorder=true
cherniavskii Dec 5, 2022
96a23a7
apply border color using `withBorderColor` class
cherniavskii Dec 6, 2022
2545feb
remove unused ownerState
cherniavskii Dec 6, 2022
9b287b9
rename `showCellRightBorder` prop
cherniavskii Dec 8, 2022
9f0a370
Merge branch 'next' into showCellRightBorder-right-pinned-columns
cherniavskii Dec 8, 2022
d9537fb
rename `showColumnRightBorder` prop
cherniavskii Dec 8, 2022
709a2e9
document renamed CSS class
cherniavskii Dec 8, 2022
8b5131e
remove 'withBorder' class usages
cherniavskii Dec 12, 2022
4f14f00
avoid using borderColor
cherniavskii Dec 12, 2022
938da8e
Merge branch 'next' into showCellRightBorder-right-pinned-columns
cherniavskii Dec 12, 2022
09856d8
fix visual regressions
cherniavskii Dec 12, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
GridVirtualScrollerContent,
GridVirtualScrollerRenderZone,
useGridVirtualScroller,
getBorderColor,
} from '@mui/x-data-grid/internals';
import { useGridApiContext } from '../hooks/utils/useGridApiContext';
import { useGridRootProps } from '../hooks/utils/useGridRootProps';
Expand Down Expand Up @@ -84,6 +85,7 @@ const useUtilityClasses = (ownerState: OwnerState) => {

interface VirtualScrollerPinnedColumnsProps {
side: GridPinnedPosition;
showCellRightBorder: boolean;
}

// Inspired by https://github.com/material-components/material-components-ios/blob/bca36107405594d5b7b16265a5b0ed698f85a5ee/components/Elevation/src/UIColor%2BMaterialElevation.m#L61
Expand Down Expand Up @@ -136,6 +138,10 @@ const VirtualScrollerPinnedColumns = styled('div', {
}),
...(ownerState.side === GridPinnedPosition.left && { left: 0, float: 'left' }),
...(ownerState.side === GridPinnedPosition.right && { right: 0, float: 'right' }),
...(ownerState.side === GridPinnedPosition.right &&
ownerState.showCellRightBorder && {
borderLeft: `1px solid ${getBorderColor(theme)}`,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is the right approach. Currently, if showCellRightBorder is on the cells receive the .MuiDataGrid-withBorder class. This class is specifically to change the border of the cells. Here we're changing the meaning of showCellRightBorder by applying a border not to the pinned cells but to their container. If the user wants to change the color of the cell border he will have to change in two places: here and in .MuiDataGrid-withBorder.

My proposal is to change .MuiDataGrid-withBorder to only set the border color, then the cells will set the border width/style on the appropriate side.

For v6 I agree to rename this prop to showCellVerticalBorder or other name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!

If the user wants to change the color of the cell border he will have to change in two places: here and in .MuiDataGrid-withBorder.

But there's more places with border color anyway:

Maybe we should add separate borderColor class and apply it everywhere border is used?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah but these other borders affect other parts, not the cells. Currently, .MuiDataGrid-withBorder only impacts the cell border.

Maybe we should add separate borderColor class and apply it everywhere border is used?

We'll have two classes for borders. In v6 I think we can unify this behavior and apply MuiDataGrid-withBorder to the grid instead, not the cells. Then, we can have a single border color. It could also be a CSS variable, to be used in color.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I've split withBorder styles into two classes:

  • withBorder that applies border-color only
  • withRightBorder that applies border-right-width and border-right-style only

I've also used withBorder to apply border color to right-pinned container.

Copy link
Member

Choose a reason for hiding this comment

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

We miss a withLeftBorder applying border-left-width and border-right-style to the cell, not to the right pinned columns container.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are 2 problems with that:

  1. I don't see a way to set showLeftBorder={true} only for first cells in the right-pinned section.

  2. Applying left border to cell instead of right pinned columns container results in double border:

    borderLeft on right-pinned columns container
    Screenshot 2022-04-04 at 19 54 15
    borderLeft on cell
    Screenshot 2022-04-04 at 19 52 52
    I didn't find a way to avoid this double border.

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've renamed showCellRightBorder to showCellVerticalBorder, showColumnRightBorder to showColumnVerticalBorder and added withBorderColor CSS class that sets the border color for both cells and column headers

}),
}));

interface DataGridProVirtualScrollerProps extends React.HTMLAttributes<HTMLDivElement> {
Expand Down Expand Up @@ -287,7 +293,10 @@ const DataGridProVirtualScroller = React.forwardRef<
<VirtualScrollerPinnedColumns
ref={leftColumns}
className={classes.leftPinnedColumns}
ownerState={{ side: GridPinnedPosition.left }}
ownerState={{
side: GridPinnedPosition.left,
showCellRightBorder: rootProps.showCellRightBorder,
}}
style={pinnedColumnsStyle}
>
{getRows({
Expand All @@ -301,7 +310,10 @@ const DataGridProVirtualScroller = React.forwardRef<
{rightRenderContext && (
<VirtualScrollerPinnedColumns
ref={rightColumns}
ownerState={{ side: GridPinnedPosition.right }}
ownerState={{
side: GridPinnedPosition.right,
showCellRightBorder: rootProps.showCellRightBorder,
}}
className={classes.rightPinnedColumns}
style={pinnedColumnsStyle}
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,28 @@ describe('<DataGridPro /> - Column pinning', () => {
expect(getColumnHeadersTextContent()).to.deep.equal(['id', '', 'Currency Pair']);
});

it('should add border to right pinned columns section when `showCellRightBorder={true}`', function test() {
if (isJSDOM) {
// Doesn't work with mocked window.getComputedStyle
this.skip();
}

render(
<div style={{ width: 300, height: 500 }}>
<TestCase showCellRightBorder initialState={{ pinnedColumns: { right: ['id'] } }} />
</div>,
);

const computedStyle = window.getComputedStyle(
document.querySelector('.MuiDataGrid-pinnedColumns--right') as HTMLElement,
);
const borderLeftColor = computedStyle.getPropertyValue('border-left-color');
const borderLeftWidth = computedStyle.getPropertyValue('border-left-width');
expect(borderLeftWidth).to.equal('1px');
// should not be transparent
expect(borderLeftColor).to.not.equal('rgba(0, 0, 0, 0)');
});

describe('props: onPinnedColumnsChange', () => {
it('should call when a column is pinned', () => {
const handlePinnedColumnsChange = spy();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import * as React from 'react';
import clsx from 'clsx';
import { unstable_composeClasses as composeClasses } from '@mui/material';
import { styled, alpha, lighten, darken } from '@mui/material/styles';
import { styled } from '@mui/material/styles';
import { getDataGridUtilityClass } from '../../constants/gridClasses';
import { useGridRootProps } from '../../hooks/utils/useGridRootProps';
import { DataGridProcessedProps } from '../../models/props/DataGridProps';
import { getBorderColor } from '../../utils/styleUtils';

type OwnerState = {
classes?: DataGridProcessedProps['classes'];
Expand All @@ -25,10 +26,7 @@ const GridColumnHeadersRoot = styled('div', {
slot: 'ColumnHeaders',
overridesResolver: (props, styles) => styles.columnHeaders,
})(({ theme }) => {
const borderColor =
theme.palette.mode === 'light'
? lighten(alpha(theme.palette.divider, 1), 0.88)
: darken(alpha(theme.palette.divider, 1), 0.68);
const borderColor = getBorderColor(theme);

return {
position: 'absolute',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import * as React from 'react';
import clsx from 'clsx';
import { unstable_composeClasses as composeClasses } from '@mui/material';
import { styled, alpha, lighten, darken } from '@mui/material/styles';
import { styled } from '@mui/material/styles';
import { getDataGridUtilityClass } from '../../constants/gridClasses';
import { useGridRootProps } from '../../hooks/utils/useGridRootProps';
import { DataGridProcessedProps } from '../../models/props/DataGridProps';
import { getBorderColor } from '../../utils/styleUtils';

export type GridFooterContainerProps = React.HTMLAttributes<HTMLDivElement>;

Expand All @@ -25,10 +26,7 @@ const GridFooterContainerRoot = styled('div', {
slot: 'FooterContainer',
overridesResolver: (props, styles) => styles.footerContainer,
})(({ theme }) => {
const borderColor =
theme.palette.mode === 'light'
? lighten(alpha(theme.palette.divider, 1), 0.88)
: darken(alpha(theme.palette.divider, 1), 0.68);
const borderColor = getBorderColor(theme);

return {
display: 'flex',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { CSSInterpolation } from '@mui/system';
import { darken, lighten, alpha, styled } from '@mui/material/styles';
import { alpha, styled } from '@mui/material/styles';
import { gridClasses } from '../../constants/gridClasses';
import { getBorderColor } from '../../utils/styleUtils';

export const GridRootStyles = styled('div', {
name: 'MuiDataGrid',
Expand Down Expand Up @@ -54,10 +55,7 @@ export const GridRootStyles = styled('div', {
styles.root,
],
})(({ theme }) => {
const borderColor =
theme.palette.mode === 'light'
? lighten(alpha(theme.palette.divider, 1), 0.88)
: darken(alpha(theme.palette.divider, 1), 0.68);
const borderColor = getBorderColor(theme);

const gridStyle: CSSInterpolation = {
flex: 1,
Expand Down
1 change: 1 addition & 0 deletions packages/grid/x-data-grid/src/internals/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,4 @@ export { createSelector } from '../utils/createSelector';
export { findParentElementFromClassName } from '../utils/domUtils';
export { isNavigationKey } from '../utils/keyboardUtils';
export { clamp, isDeepEqual } from '../utils/utils';
export { getBorderColor } from '../utils/styleUtils';
7 changes: 7 additions & 0 deletions packages/grid/x-data-grid/src/utils/styleUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { darken, lighten, alpha, Theme } from '@mui/material/styles';

export function getBorderColor(theme: Theme) {
return theme.palette.mode === 'light'
? lighten(alpha(theme.palette.divider, 1), 0.88)
: darken(alpha(theme.palette.divider, 1), 0.68);
}
41 changes: 36 additions & 5 deletions packages/storybook/.storybook/preview.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import * as React from 'react';
import { createTheme, ThemeProvider as MUIThemeProvider } from '@mui/material/styles';
import { CssBaseline } from '@mui/material';
import { ThemeProvider } from 'emotion-theming';
import { LicenseInfo } from '@mui/x-data-grid-pro';
import { INITIAL_VIEWPORTS } from '@storybook/addon-viewport';
import { configureActions } from '@storybook/addon-actions';
Expand Down Expand Up @@ -44,9 +47,37 @@ export const parameters = {
};

export const decorators = [
(Story) => (
<React.StrictMode>
<Story />
</React.StrictMode>
),
(Story, context) => {
const theme = createTheme({
palette: {
mode: context.globals.theme,
},
});
return (
<React.StrictMode>
<MUIThemeProvider theme={theme}>
{/* See https://github.com/mui/material-ui/issues/24282#issuecomment-952211989 */}
<ThemeProvider theme={theme}>
<CssBaseline />
<Story />
</ThemeProvider>
</MUIThemeProvider>
</React.StrictMode>
);
},
];

export const globalTypes = {
theme: {
name: 'Theme',
description: 'MUI theme',
defaultValue: 'light',
toolbar: {
icon: 'circlehollow',
// Array of plain string values or MenuItem shape (see below)
items: ['light', 'dark'],
// Property that specifies if the name of the item will be displayed
showName: true,
},
},
};
21 changes: 21 additions & 0 deletions packages/storybook/src/stories/grid-columns.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -448,3 +448,24 @@ export function PinnedColumnWithCheckboxSelectionSnap() {
</div>
);
}

export function PinnedColumnsWithCellRightBorder() {
const { data } = useDemoData({
dataSet: 'Commodity',
rowLength: 15,
maxColumns: 6,
});

const columns = data.columns.map((column) => ({ ...column, width: 150 }));

return (
<div style={{ height: 400, width: 600 }}>
<DataGridPro
{...data}
columns={columns}
initialState={{ pinnedColumns: { left: ['desk'], right: ['quantity'] } }}
showCellRightBorder
/>
</div>
);
}