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

[DataGrid] Column header design updates #14293

Merged
merged 18 commits into from
Sep 3, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
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 @@ -41,12 +41,9 @@ const GridAggregationFunctionLabel = styled('div', {
})<{ ownerState: OwnerState }>(({ theme }) => {
return {
fontSize: theme.typography.caption.fontSize,
lineHeight: theme.typography.caption.fontSize,
position: 'absolute',
bottom: 4,
fontWeight: theme.typography.fontWeightMedium,
color: (theme.vars || theme).palette.primary.dark,
textTransform: 'uppercase',
lineHeight: 'normal',
Copy link
Member

@MBilalShafi MBilalShafi Aug 28, 2024

Choose a reason for hiding this comment

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

I'm wondering if there is a possibility some users customizing it using theme.*.* variables. What do you think? 🤔

Copy link
Member Author

@KenanYusuf KenanYusuf Aug 28, 2024

Choose a reason for hiding this comment

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

I'm purposefully deviating from the theme.typography.caption.lineHeight value here because it is too large. normal line-height should keep the spacing looking ok between the column header title and aggregation label I think.

If users want to override it, they have the class I guess?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, but I'm only worried if some existing users' styles would break with this.
It's obviously not very likely but theoretically, some users may be using theme.typography.caption.lineHeight to override this style.

It's a nitpick though. Feel free to merge it in current shape.

color: theme.palette.text.secondary,
marginTop: -1,
};
});

Expand Down
16 changes: 8 additions & 8 deletions packages/x-data-grid-pro/src/tests/columns.DataGridPro.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -166,11 +166,11 @@ describe('<DataGridPro /> - Columns', () => {
await microtasks();
expect(onColumnWidthChange.callCount).to.be.at.least(2);
const widthArgs = onColumnWidthChange.args.map((arg) => arg[0].width);
const isWidth116Present = widthArgs.some((width) => width === 116);
expect(isWidth116Present).to.equal(true);
const isWidth114Present = widthArgs.some((width) => width === 114);
expect(isWidth114Present).to.equal(true);
const colDefWidthArgs = onColumnWidthChange.args.map((arg) => arg[0].colDef.width);
const isColDefWidth116Present = colDefWidthArgs.some((width) => width === 116);
expect(isColDefWidth116Present).to.equal(true);
const isColDefWidth114Present = colDefWidthArgs.some((width) => width === 114);
expect(isColDefWidth114Present).to.equal(true);
});

it('should not affect other cell elements that are not part of the main DataGrid instance', () => {
Expand Down Expand Up @@ -473,7 +473,7 @@ describe('<DataGridPro /> - Columns', () => {
render(<Test rows={rows} columns={columns} />);
await apiRef.current.autosizeColumns();
await microtasks();
expect(getWidths()).to.deep.equal([213, 235]);
expect(getWidths()).to.deep.equal([211, 233]);
});

it('should work through double-clicking the separator', async () => {
Expand All @@ -483,14 +483,14 @@ describe('<DataGridPro /> - Columns', () => {
)[1];
fireEvent.doubleClick(separator);
await microtasks();
expect(getWidths()).to.deep.equal([100, 235]);
expect(getWidths()).to.deep.equal([100, 233]);
});

it('should work on mount', async () => {
render(<Test rows={rows} columns={columns} autosizeOnMount />);
await microtasks(); /* first effect after render */
await microtasks(); /* async autosize operation */
expect(getWidths()).to.deep.equal([213, 235]);
expect(getWidths()).to.deep.equal([211, 233]);
});

describe('options', () => {
Expand All @@ -506,7 +506,7 @@ describe('<DataGridPro /> - Columns', () => {
});

it('.includeHeaders works', async () => {
await autosize({ includeHeaders: true }, [213, 235]);
await autosize({ includeHeaders: true }, [211, 233]);
});

it('.includeOutliers works', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export const ColumnHeaderMenuIcon = React.memo((props: ColumnHeaderMenuIconProps
id={columnMenuButtonId}
{...rootProps.slotProps?.baseIconButton}
>
<rootProps.slots.columnMenuIcon fontSize="small" />
<rootProps.slots.columnMenuIcon fontSize="inherit" />
</rootProps.slots.baseIconButton>
</rootProps.slots.baseTooltip>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ function GridColumnHeaderSeparatorRaw(props: GridColumnHeaderSeparatorProps) {
// eslint-disable-next-line jsx-a11y/click-events-have-key-events,jsx-a11y/no-static-element-interactions
<div
className={classes.root}
style={{ minHeight: height, opacity: rootProps.showColumnVerticalBorder ? 0 : 1 }}
style={{ minHeight: height, opacity: rootProps.showColumnVerticalBorder ? 0 : undefined }}
MBilalShafi marked this conversation as resolved.
Show resolved Hide resolved
{...other}
onClick={stopClick}
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const GridColumnHeaderTitleRoot = styled('div', {
overflow: 'hidden',
whiteSpace: 'nowrap',
fontWeight: 'var(--unstable_DataGrid-headWeight)',
lineHeight: 'normal',
});

const ColumnHeaderInnerTitle = React.forwardRef<
Expand Down
110 changes: 71 additions & 39 deletions packages/x-data-grid/src/components/containers/GridRootStyles.ts
Copy link
Member

Choose a reason for hiding this comment

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

Side note: Sometimes Argos diff surprises me – it doesn't pick up the separator 😅

Screen.Recording.2024-08-23.at.23.36.55.mov

Copy link
Member

Choose a reason for hiding this comment

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

Also, this PR warrants a minor release.

Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,6 @@ function getBorderColor(theme: Theme) {
return darken(alpha(theme.palette.divider, 1), 0.68);
}

const columnHeadersStyles = {
[`.${c.columnSeparator}, .${c['columnSeparator--resizing']}`]: {
visibility: 'visible',
width: 'auto',
},
};

const columnHeaderStyles = {
[`& .${c.iconButtonContainer}`]: {
visibility: 'visible',
Expand All @@ -45,6 +38,12 @@ const columnHeaderStyles = {
},
};

const separatorIconDragStyles = {
width: 3,
rx: 1.5,
x: 10.5,
};

// Emotion thinks it knows better than us which selector we should use.
// https://github.com/emotion-js/emotion/issues/1105#issuecomment-1722524968
const ignoreSsrWarning =
Expand Down Expand Up @@ -188,6 +187,10 @@ export const GridRootStyles = styled('div', {
},
};

const focusOutlineWidth = 1;
const columnSeparatorTargetSize = 10;
const columnSeparatorOffset = -5;

const gridStyle: CSSInterpolation = {
'--unstable_DataGrid-radius': typeof radius === 'number' ? `${radius}px` : radius,
'--unstable_DataGrid-headWeight': t.typography.fontWeightMedium,
Expand Down Expand Up @@ -264,7 +267,6 @@ export const GridRootStyles = styled('div', {
},
[`& .${c.columnHeader}, & .${c.cell}`]: {
WebkitTapHighlightColor: 'transparent',
lineHeight: null,
padding: '0 10px',
boxSizing: 'border-box',
},
Expand All @@ -273,12 +275,35 @@ export const GridRootStyles = styled('div', {
t.vars
? `rgba(${t.vars.palette.primary.mainChannel} / 0.5)`
: alpha(t.palette.primary.main, 0.5)
} 1px`,
outlineWidth: 1,
outlineOffset: -1,
} ${focusOutlineWidth}px`,
outlineOffset: focusOutlineWidth * -1,
},
[`& .${c.columnHeader}:focus, & .${c.cell}:focus`]: {
outline: `solid ${t.palette.primary.main} 1px`,
outline: `solid ${t.palette.primary.main} ${focusOutlineWidth}px`,
outlineOffset: focusOutlineWidth * -1,
},
// Hide the column separator when:
// - the column is focused and has an outline
// - the next column is focused and has an outline
// - the column has a right border
// - the next column has a right border
[`& .${c.columnHeader}:focus,
& .${c.columnHeader}:focus-within,
& .${c.columnHeader}:has(+ .${c.columnHeader}:focus),
& .${c.columnHeader}:has(+ .${c.columnHeader}:focus-within),
& .${c['columnHeader--withRightBorder']},
& .${c.columnHeader}:has(+ .${c['columnHeader--withRightBorder']})`]: {
[`& .${c.columnSeparator}`]: {
opacity: 0,
'@media (hover: none)': {
opacity: 1,
color: (t.vars || t).palette.primary.main,
},
},
// Show resizable separators again when the column is hovered
[`& .${c['columnSeparator--resizable']}:hover`]: {
opacity: 1,
},
},
[`&.${c['root--noToolbar']} [aria-rowindex="1"] [aria-colindex="1"]`]: {
borderTopLeftRadius: 'calc(var(--unstable_DataGrid-radius) - 1px)',
Expand All @@ -299,7 +324,7 @@ export const GridRootStyles = styled('div', {
display: 'flex',
alignItems: 'center',
},
[`& .${c['columnHeader--last']}`]: {
[`& .${c['virtualScroller--hasScrollX']} .${c['columnHeader--last']}`]: {
overflow: 'hidden',
},
KenanYusuf marked this conversation as resolved.
Show resolved Hide resolved
[`& .${c['columnHeader--sorted']} .${c.iconButtonContainer}, & .${c['columnHeader--filtered']} .${c.iconButtonContainer}`]:
Expand All @@ -316,12 +341,11 @@ export const GridRootStyles = styled('div', {
[`& .${c.columnHeaderTitleContainer}`]: {
display: 'flex',
alignItems: 'center',
gap: t.spacing(0.25),
minWidth: 0,
flex: 1,
whiteSpace: 'nowrap',
overflow: 'hidden',
// to anchor the aggregation label
position: 'relative',
},
[`& .${c.columnHeaderTitleContainerContent}`]: {
overflow: 'hidden',
Expand All @@ -346,16 +370,13 @@ export const GridRootStyles = styled('div', {
{
flexDirection: 'row-reverse',
},
[`& .${c['columnHeader--alignCenter']} .${c.menuIcon}, & .${c['columnHeader--alignRight']} .${c.menuIcon}`]:
{
marginRight: 'auto',
marginLeft: -6,
},
[`& .${c['columnHeader--alignRight']} .${c.menuIcon}, & .${c['columnHeader--alignRight']} .${c.menuIcon}`]:
{
marginRight: 'auto',
marginLeft: -10,
},
[`& .${c['columnHeader--alignCenter']} .${c.menuIcon}`]: {
marginLeft: 'auto',
},
[`& .${c['columnHeader--alignRight']} .${c.menuIcon}`]: {
marginRight: 'auto',
marginLeft: -5,
},
[`& .${c['columnHeader--moving']}`]: {
backgroundColor: (t.vars || t).palette.action.hover,
},
Expand All @@ -365,59 +386,70 @@ export const GridRootStyles = styled('div', {
background: 'var(--DataGrid-pinnedBackground)',
},
[`& .${c.columnSeparator}`]: {
visibility: 'hidden',
position: 'absolute',
overflow: 'hidden',
zIndex: 3,
display: 'flex',
flexDirection: 'column',
justifyContent: 'center',
alignItems: 'center',
maxWidth: columnSeparatorTargetSize,
color: borderColor,
},
[`& .${c.columnHeaders}`]: {
width: 'var(--DataGrid-rowWidth)',
},
'@media (hover: hover)': {
[`& .${c.columnHeaders}:hover`]: columnHeadersStyles,
[`& .${c.columnHeader}:hover`]: columnHeaderStyles,
[`& .${c.columnHeader}:not(.${c['columnHeader--sorted']}):hover .${c.sortIcon}`]: {
opacity: 0.5,
},
},
'@media (hover: none)': {
[`& .${c.columnHeaders}`]: columnHeadersStyles,
[`& .${c.columnHeader}`]: columnHeaderStyles,
},
[`& .${c['columnSeparator--sideLeft']}`]: {
left: -12,
left: columnSeparatorOffset,
},
[`& .${c['columnSeparator--sideRight']}`]: {
right: -12,
right: columnSeparatorOffset,
},
[`& .${c['columnHeader--withRightBorder']} .${c['columnSeparator--sideLeft']}`]: {
left: columnSeparatorOffset - 0.5,
},
[`& .${c['columnHeader--withRightBorder']} .${c['columnSeparator--sideRight']}`]: {
right: columnSeparatorOffset - 0.5,
},
[`& .${c['columnSeparator--resizable']}`]: {
cursor: 'col-resize',
touchAction: 'none',
'&:hover': {
color: (t.vars || t).palette.text.primary,
// Reset on touch devices, it doesn't add specificity
// Always appear as draggable on touch devices
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻

'@media (hover: none)': {
[`& .${c.iconSeparator} rect`]: separatorIconDragStyles,
color: borderColor,
},
[`&:hover, &.${c['columnSeparator--resizing']}`]: {
color: (t.vars || t).palette.primary.main,
KenanYusuf marked this conversation as resolved.
Show resolved Hide resolved
[`& .${c.iconSeparator} rect`]: separatorIconDragStyles,
'@media (hover: none)': {
color: borderColor,
},
},
[`&.${c['columnSeparator--resizing']}`]: {
color: (t.vars || t).palette.text.primary,
},
'& svg': {
pointerEvents: 'none',
},
},
[`& .${c.iconSeparator}`]: {
color: 'inherit',
transition: t.transitions.create(['color', 'width'], {
duration: t.transitions.duration.shortest,
}),
},
[`& .${c.menuIcon}`]: {
width: 0,
visibility: 'hidden',
fontSize: 20,
marginRight: -10,
marginRight: -5,
display: 'flex',
alignItems: 'center',
},
Expand Down Expand Up @@ -511,8 +543,8 @@ export const GridRootStyles = styled('div', {
boxShadow: t.shadows[2],
backgroundColor: (t.vars || t).palette.background.paper,
'&:focus-within': {
outline: `solid ${(t.vars || t).palette.primary.main} 1px`,
outlineOffset: '-1px',
outline: `${focusOutlineWidth}px solid ${(t.vars || t).palette.primary.main}`,
outlineOffset: focusOutlineWidth * -1,
},
},
[`& .${c['row--editing']}`]: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,13 @@ const useUtilityClasses = (
loadingOverlayVariant: GridLoadingOverlayVariant | null,
) => {
const { classes } = ownerState;

const slots = {
root: [
'main',
dimensions.rightPinnedWidth > 0 && 'main--hasPinnedRight',
loadingOverlayVariant === 'skeleton' && 'main--hasSkeletonLoadingOverlay',
],
scroller: ['virtualScroller'],
scroller: ['virtualScroller', dimensions.hasScrollX && 'virtualScroller--hasScrollX'],
};

return composeClasses(slots, getDataGridUtilityClass, classes);
Expand Down
6 changes: 6 additions & 0 deletions packages/x-data-grid/src/constants/gridClasses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,11 @@ export interface GridClasses {
* Styles applied to the virtualization container.
*/
virtualScroller: string;
/**
* Styles applied to the virtualization container when it is scrollable in the horizontal direction.
* @ignore - do not document.
*/
'virtualScroller--hasScrollX': string;
/**
* Styles applied to the virtualization content.
*/
Expand Down Expand Up @@ -770,6 +775,7 @@ export const gridClasses = generateUtilityClasses<GridClassKey>('MuiDataGrid', [
'toolbarContainer',
'toolbarFilterList',
'virtualScroller',
'virtualScroller--hasScrollX',
'virtualScrollerContent',
'virtualScrollerContent--overflowed',
'virtualScrollerRenderZone',
Expand Down
5 changes: 4 additions & 1 deletion packages/x-data-grid/src/material/icons/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@ export const GridColumnIcon = createSvgIcon(
'ColumnIcon',
);

export const GridSeparatorIcon = createSvgIcon(<path d="M11 19V5h2v14z" />, 'Separator');
export const GridSeparatorIcon = createSvgIcon(
<rect width="1" height="24" x="11.5" rx="0.5" />,
'Separator',
);

export const GridViewHeadlineIcon = createSvgIcon(
<path d="M4 15h16v-2H4v2zm0 4h16v-2H4v2zm0-8h16V9H4v2zm0-6v2h16V5H4z" />,
Expand Down