-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: highlight editable cells on hover #1087
Changes from 2 commits
4ce98a4
3f328cc
c7a280c
c3fb6f0
50b9c12
47b5bdc
bd79761
2a8c8c6
d92c2b2
fdec08f
3c13d72
b3658bf
9be0585
a6eec09
257e758
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,8 @@ export interface GridStyle { | |
firstRowMessageCss?: Properties; | ||
/** Applied on hover if a row has a rowLink/onClick set. */ | ||
rowHoverColor?: Palette | "none"; | ||
/** Applied on hover to a cell TextFieldBase */ | ||
rowEditableCellBorderColor?: Palette; | ||
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. @0ces afaict we're not really setting this on "the cell", but instead pushing it down into the child |
||
/** Applied on hover of a row */ | ||
nonHeaderRowHoverCss?: Properties; | ||
/** Default content to put into an empty cell */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,6 +88,7 @@ function RowImpl<R extends Kinded, S>(props: RowProps<R>): ReactElement { | |
const sortOn = tableState.sortConfig?.on; | ||
|
||
const revealOnRowHoverClass = "revealOnRowHover"; | ||
const editableOnRowHoverClass = "editableOnRowHover"; | ||
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. @0ces I'm still reading the PR (on mobile 😅) but can't this special class name go away with the PC based approach? I.e. my primary concern with the original PR was that TextField was being told "don't show your border" ... And then later GridTable had a bunch of special "reach into TextFields DOM and tell it to do something else", which is a brittle approach/breaks encapsulation. ...I suppose you're staying with the special class name to take advantage of :hover psuedo-selector? I'm not totally against using the :hover selector, but I'd like any "what does TextFields border look like when borderless but hovered" to be owned/live directly in TextField, and not GridTable. Wdyt, in terms of approaches to do that? 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. @stephenh yeah, I'm keeping that class just to propagate the :hover state to the cells when hovering the rows and not the actual field, I really don't know a better way of doing the propagation, also as this is just CSS just for it not to cause a ton of re-renders. 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. @0ces right, but can we get the hover psuedo-selectors at least placed within the TextField class itself? I.e. GridTable will only "turn on" the hiber behavior, but the actual render details are in the TF. 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. I can give it a try |
||
|
||
const showRowHoverColor = !reservedRowKinds.includes(row.kind) && !omitRowHover && style.rowHoverColor !== "none"; | ||
|
||
|
@@ -123,6 +124,11 @@ function RowImpl<R extends Kinded, S>(props: RowProps<R>): ReactElement { | |
[` > .${revealOnRowHoverClass} > *`]: Css.vh.$, | ||
[`:hover > .${revealOnRowHoverClass} > *`]: Css.vv.$, | ||
}, | ||
...{ | ||
[`:hover > .${editableOnRowHoverClass} .textFieldBaseWrapper`]: Css.px1.br4.ba.bc( | ||
style.rowEditableCellBorderColor ?? Palette.Blue300, | ||
).$, | ||
}, | ||
...(isLastKeptRow && Css.addIn("&>*", style.keptLastRowCss).$), | ||
}; | ||
|
||
|
@@ -210,7 +216,19 @@ function RowImpl<R extends Kinded, S>(props: RowProps<R>): ReactElement { | |
onDragOver: onDragOverDebounced, | ||
}; | ||
|
||
const maybeContent = applyRowFn(column as GridColumnWithId<R>, row, rowApi, level, isExpanded, dragData); | ||
const cellId = `${row.kind}_${row.id}_${column.id}`; | ||
const applyCellHighlight = cellHighlight && !!column.id && !isHeader && !isTotals; | ||
const isCellActive = tableState.activeCellId === cellId; | ||
|
||
const maybeContent = applyRowFn( | ||
column as GridColumnWithId<R>, | ||
row, | ||
rowApi, | ||
level, | ||
isExpanded, | ||
isCellActive, | ||
dragData, | ||
); | ||
|
||
// Only use the `numExpandedColumns` as the `colspan` when rendering the "Expandable Header" | ||
currentColspan = | ||
|
@@ -220,6 +238,7 @@ function RowImpl<R extends Kinded, S>(props: RowProps<R>): ReactElement { | |
? numExpandedColumns + 1 | ||
: 1; | ||
const revealOnRowHover = isGridCellContent(maybeContent) ? maybeContent.revealOnRowHover : false; | ||
const editableOnRowHover = isGridCellContent(maybeContent) ? maybeContent.editableOnHover : false; | ||
|
||
const canSortColumn = | ||
(sortOn === "client" && column.clientSideSort !== false) || | ||
|
@@ -274,11 +293,6 @@ function RowImpl<R extends Kinded, S>(props: RowProps<R>): ReactElement { | |
|
||
// This relies on our column sizes being defined in pixel values, which is currently true as we calculate to pixel values in the `useSetupColumnSizes` hook | ||
minStickyLeftOffset += maybeSticky === "left" ? parseInt(columnSizes[columnIndex].replace("px", ""), 10) : 0; | ||
|
||
const cellId = `${row.kind}_${row.id}_${column.id}`; | ||
const applyCellHighlight = cellHighlight && !!column.id && !isHeader && !isTotals; | ||
const isCellActive = tableState.activeCellId === cellId; | ||
|
||
// Note that it seems expensive to calc a per-cell class name/CSS-in-JS output, | ||
// vs. setting global/table-wide CSS like `style.cellCss` on the root grid div with | ||
// a few descendent selectors. However, that approach means the root grid-applied | ||
|
@@ -334,8 +348,15 @@ function RowImpl<R extends Kinded, S>(props: RowProps<R>): ReactElement { | |
})`, | ||
}; | ||
|
||
const cellClassNames = revealOnRowHover ? revealOnRowHoverClass : undefined; | ||
const cellClassNames = [ | ||
...(revealOnRowHover ? [revealOnRowHoverClass] : []), | ||
...(editableOnRowHover && (isCellActive || !tableState.activeCellId) ? [editableOnRowHoverClass] : []), | ||
].join(" "); | ||
|
||
const cellOnHover = | ||
isGridCellContent(maybeContent) && maybeContent.editableOnHover | ||
? (enter: boolean) => (enter ? api.setActiveCellId(cellId) : api.setActiveCellId(undefined)) | ||
: undefined; | ||
const cellOnClick = applyCellHighlight ? () => api.setActiveCellId(cellId) : undefined; | ||
const tooltip = isGridCellContent(maybeContent) ? maybeContent.tooltip : undefined; | ||
|
||
|
@@ -348,7 +369,17 @@ function RowImpl<R extends Kinded, S>(props: RowProps<R>): ReactElement { | |
? rowClickRenderFn(as, api, currentColspan) | ||
: defaultRenderFn(as, currentColspan); | ||
|
||
return renderFn(columnIndex, cellCss, content, row, rowStyle, cellClassNames, cellOnClick, tooltip); | ||
return renderFn( | ||
columnIndex, | ||
cellCss, | ||
content, | ||
row, | ||
rowStyle, | ||
cellClassNames, | ||
cellOnClick, | ||
cellOnHover, | ||
tooltip, | ||
); | ||
}) | ||
)} | ||
</RowTag> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,8 @@ export type GridCellContent = { | |
revealOnRowHover?: true; | ||
/** Tooltip to add to a cell */ | ||
tooltip?: ReactNode; | ||
/** Allows cell to be editable when hovering, and also highlights the field on hover */ | ||
editableOnHover?: true; | ||
0ces marked this conversation as resolved.
Show resolved
Hide resolved
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. @0ces cc @apattersonATX-HB do we want this to be a per-cell flag? Kinda thinking that the entire table should be consistent about "all columns/cells are 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. Setting at the table makes sense. Maybe have an override on the cell/column level? 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. If we can articulate a reason why a cell/column legitimately should not follow the "hover draws borders" behavior, yeah we could, but otherwise I'd prefer not to provide the option as a strong recommendation to the engineer/UX designer that "you should not be doing this". (I.e. without a strong Figma-based design system driving consistency from the UX side, we've basically been using Beam's "opinionated APIs" to drive consistency back to the design team.) 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. Nit/suggestion: 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. Ah damn, I was pushing towards "each table itself should at least be consistent" (instead of a per cell decision), but ngl I like Brandon's push about "do they (design) just want all tables in BP to act this way?" Because that is actually easier for us to globally roll out, and per Brandon's point likely (surely?) better for users (consistency), to just have our tables "always do that" (whatever the latest/greatest UX is), vs. picking tables onsey-twosy that do/do not get the special behavior. 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. AFAIK this is a new design thing, I can ask design if this will be the new standard, I tried not to impose massive changes, this is the figma that I've been following for this PR: https://www.figma.com/design/xQ3iNLHJCFEJeOi7boONY4/Q1-2024-%7C-Dynamic-Schedules?node-id=11225-33807&t=BgAyiY5hjDNYwF9v-1 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. @0ces cool, feel free to loop me into any design discussions 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. Yes, this is supposed to be the new global standard. 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. @stephenh the feedback that I got from design: As I understand it design does want to make this to make this the new standard but with a slow rollout 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. What does this comment mean by "non-table cells"? |
||
}; | ||
|
||
/** Allows rendering a specific cell. */ | ||
|
@@ -45,19 +47,23 @@ export type RenderCellFn<R extends Kinded> = ( | |
rowStyle: RowStyle<R> | undefined, | ||
classNames: string | undefined, | ||
onClick: VoidFunction | undefined, | ||
onHover: ((enter: boolean) => void) | undefined, | ||
tooltip: ReactNode | undefined, | ||
) => ReactNode; | ||
|
||
/** Renders our default cell element, i.e. if no row links and no custom renderCell are used. */ | ||
export const defaultRenderFn: (as: RenderAs, colSpan: number) => RenderCellFn<any> = | ||
(as: RenderAs, colSpan) => (key, css, content, row, rowStyle, classNames: string | undefined, onClick, tooltip) => { | ||
(as: RenderAs, colSpan) => | ||
(key, css, content, row, rowStyle, classNames: string | undefined, onClick, onHover, tooltip) => { | ||
const Cell = as === "table" ? "td" : "div"; | ||
return ( | ||
<Cell | ||
key={key} | ||
css={{ ...css, ...Css.cursor("default").$ }} | ||
className={classNames} | ||
onClick={onClick} | ||
onMouseEnter={() => onHover?.(true)} | ||
onMouseLeave={() => onHover?.(false)} | ||
{...(as === "table" && { colSpan })} | ||
> | ||
{content} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -229,6 +229,7 @@ export function TextFieldBase<X extends Only<TextFieldXss, X>>(props: TextFieldB | |
...(multiline ? Css.fdc.aifs.gap2.$ : Css.if(wrap === false).truncate.$), | ||
...xss, | ||
}} | ||
className="textFieldBaseWrapper" | ||
data-readonly="true" | ||
{...tid} | ||
> | ||
|
@@ -259,6 +260,7 @@ export function TextFieldBase<X extends Only<TextFieldXss, X>>(props: TextFieldB | |
...(errorMsg && !inputProps.disabled ? fieldStyles.error : {}), | ||
...Css.if(multiline).aifs.oh.mhPx(textAreaMinHeight).$, | ||
}} | ||
className="textFieldBaseWrapper" | ||
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. I could envision eventually needing other non-
To have the names align to the general capability we're adding, which is that should a component have |
||
{...hoverProps} | ||
ref={inputWrapRef as any} | ||
onClick={unfocusedPlaceholder ? handleUnfocusedPlaceholderClick : undefined} | ||
|
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.
Is this enum supposed to be actively changing the coloring/styling/behavior of the style, or is it just a dummy enum to show what a select field looks like in the table?
If it's just a dummy enum, can we make the enum values something like
Foo = "Foo"
andBar = "Bar
"` just to highlight their meaningless-ness?