-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Changes from 11 commits
77250ac
6867cc1
ec97638
59e4b1f
c30292b
dc03720
89154b3
bfcb7f6
0ef49b3
81a44dd
14345bb
129f30f
035e9cf
b194f5d
4303b0e
d18c885
7a37ce8
8051e7c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.movThere was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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', | ||
|
@@ -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 = | ||
|
@@ -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, | ||
|
@@ -264,7 +267,6 @@ export const GridRootStyles = styled('div', { | |
}, | ||
[`& .${c.columnHeader}, & .${c.cell}`]: { | ||
WebkitTapHighlightColor: 'transparent', | ||
lineHeight: null, | ||
padding: '0 10px', | ||
boxSizing: 'border-box', | ||
}, | ||
|
@@ -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)', | ||
|
@@ -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}`]: | ||
|
@@ -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', | ||
|
@@ -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, | ||
}, | ||
|
@@ -365,47 +386,55 @@ 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||
}, | ||
|
@@ -417,7 +446,7 @@ export const GridRootStyles = styled('div', { | |
width: 0, | ||
visibility: 'hidden', | ||
fontSize: 20, | ||
marginRight: -10, | ||
marginRight: -5, | ||
display: 'flex', | ||
alignItems: 'center', | ||
}, | ||
|
@@ -511,8 +540,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']}`]: { | ||
|
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.
I'm wondering if there is a possibility some users customizing it using
theme.*.*
variables. What do you think? 🤔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.
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?
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.
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.