feat: highlight editable cells on hover #1087

0ces · 2024-11-18T17:41:10Z

No description provided.

apattersonATX-HB

Seeing some issues here:

Screen.Recording.2024-11-19.at.1.48.13.PM.mov

Shouldn't be any layout shifts on hover
When the field is changed and then re-entered it's formatting is lost

Followup question: How do you plan on implementing the changed value highlight?

src/components/Table/GridTable.tsx

src/components/Table/components/cell.tsx

apattersonATX-HB · 2024-11-21T18:07:51Z

src/components/Table/components/Row.tsx

+    ...{
+      [`.${editableOnRowHoverClass} .textFieldBaseWrapper`]: Css.pl1.mlPx(-8).br4.ba.bcTransparent.bgTransparent.$,
+      [`.${editableOnRowHoverClass} .textFieldBaseWrapper > *`]: Css.bgTransparent.$,
+      [`.${editableOnRowHoverClass}:hover .textFieldBaseWrapper`]: Css.bgBlue100.$,
+      [`:hover:not(:has(.${editableOnRowHoverClass}:hover)) > .${editableOnRowHoverClass} .textFieldBaseWrapper, .${editableOnRowHoverClass}:hover .textFieldBaseWrapper`]:
+        Css.ba.bc(style.rowEditableCellBorderColor ?? Palette.Blue300).$,
+    },


i think this is whats causing the weird focus change issue we talked about. Just leave a comment about it and the date fields

stephenh · 2024-11-22T16:59:05Z

src/components/Table/components/Row.tsx

+    ...{
+      [`.${editableOnRowHoverClass} .textFieldBaseWrapper`]: Css.pl1.mlPx(-8).br4.ba.bcTransparent.bgTransparent.$,
+      [`.${editableOnRowHoverClass} .textFieldBaseWrapper > *`]: Css.bgTransparent.$,
+      [`.${editableOnRowHoverClass}:hover .textFieldBaseWrapper`]: Css.bgBlue100.$,


@0ces cc @apattersonATX-HB (sorry, I'm just getting to this PR) this is a ~fairly intrusive way to "reach into" the text field and muck with its internal state / styling / etc...

Like it gets the job done, but a maintainer of TextField would have no idea this behavior is happening, and could unwittingly break it while updating/maintaining TextField.

I would generally/greatly prefer some sort of "signal" from GridTable to TextField to tell it "please style yourself (this way/that way)".

Granted, it would have to be a context, given we want to cross component boundaries / we're not passing props directly to TextField.

My off-the-top-of-my-head suggestion would be to use PresentationContext, which is already how GridTable will broadcast to child/cell components "btw you're in a table,k you should render slightly differently".

Can we add something like that, for this ask / hopefully in this PR (it seems like a quick thing to try/experiment with)? Like a ... PresentationContext.visuallyEditable? That would kinda match the current visuallyDisabled prop...

Actually it looks like GridTable is already setting PresentationContext.borderless: true, to tell components "hide your borders", and it seems like the real ask here is tweaking/adding a borderless: "until-hovered" sort of thing ...

Or maybe having the GridTable flip borderless: true / false based on when the row itself is hovered?

Granted, context changes mean a React re-render to change the flag from true/false, instead of just doing the :hover ... but I think that's probably fine?

stephenh · 2024-11-22T17:01:13Z

src/components/Table/components/cell.tsx

@@ -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 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 editableOnHover yes/no"...

Setting at the table makes sense. Maybe have an override on the cell/column level?

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.)

stephenh · 2024-11-22T17:04:17Z

src/components/Table/TableStyles.tsx

@@ -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;


@0ces afaict we're not really setting this on "the cell", but instead pushing it down into the child TextField's DOM? If so, maybe it shouldn't really be a "table style" and instead each component can know how to style itself has "editable" or "borderless: unless-hovered" or what not.

bdow · 2024-11-22T17:06:25Z

src/components/Table/components/cell.tsx

@@ -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;


Nit/suggestion: editableOnHover doesn't really tell me whats happening here. Really we're just putting a border around fields on hover. The field doesn't actually become editable on hover, it just has a border now 🤷.
Taking a step back, this feels like something that should be globally applied, not just for one or two tables with inputs. I would request more design direction here. The issue with Blueprint is that every page is so different. This is just going to add to that problem.
Also, when a user uses their tab key to go between fields, then we don't get the same "bordered" treatment, unless they're hovering that row. As this PR stands, it's creating an inconsistent experience that I don't believe provides a better UX for our users. I would ask Design to fully flush this idea out, and figure out if this should be the global pattern. If not the global pattern, have rules that define when to use this pattern vs not. We should be doing this as it is best for our users.

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.

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

@0ces cool, feel free to loop me into any design discussions

Yes, this is supposed to be the new global standard.

@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

What does this comment mean by "non-table cells"?

stephenh

Would like to see some attempts/iteration on a PresentationContext-based approach, changing borderless: true to "something something hover" before this lands. Happy to chat/VC about it/approaches.

apattersonATX-HB · 2024-11-25T16:29:03Z

Would like to see some attempts/iteration on a PresentationContext-based approach, changing borderless: true to "something something hover" before this lands. Happy to chat/VC about it/approaches.

+1 to this. Still good work here @0ces! I found I was able to fix a few other issues by updating the PresentationContext through correctly using getTableStyles so we should start re-training muscles to look at that first before adjusting the cell through column config 🙏🏽

stephenh · 2024-12-02T16:21:53Z

src/components/Table/components/Row.tsx

@@ -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";


@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?

@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.

@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.

I can give it a try

stephenh

(Still reading, posting a few comments while in meetings)

stephenh · 2024-12-02T19:58:20Z

src/components/Table/GridTable.stories.tsx

+    },
+  ]);
+
+  const handleCellChange = (rowId: string, field: keyof EditableRowData["data"], value: any) => {


I know this is just a story, but can we rename this to setRow , which better describes what it does? Also would prefer a function.

Since this story is a react component I'll change it to be a useCallback

stephenh · 2024-12-02T20:00:28Z

src/components/Table/GridTable.stories.tsx

+
+enum EditableRowStatus {
+  Active = "Active",
+  Inactive = "Inactive",


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" and Bar = "Bar"` just to highlight their meaningless-ness?

src/components/Table/TableStyles.tsx

src/components/Table/components/Row.tsx

src/inputs/TextFieldBase.tsx

src/components/Table/GridTable.stories.tsx

stephenh

Few more asks for in-source comments, removing a few now-unused (afaict) lines, and maybe renaming the special className consts, but other lgtm!

The hover-unless-other-cell-hovered selector is neat, and makes sense; I think we've arrived at a pretty great solution, that still leverages :hover selectors (something I was personally willing to give up on, but you've made work well), while keeping the components in control of their own styling. Thanks!

src/components/PresentationContext.tsx

src/components/Table/components/Row.tsx

src/inputs/TextFieldBase.tsx

stephenh · 2024-12-03T01:04:15Z

src/inputs/TextFieldBase.tsx

@@ -259,6 +274,8 @@ export function TextFieldBase<X extends Only<TextFieldXss, X>>(props: TextFieldB
                ...(errorMsg && !inputProps.disabled ? fieldStyles.error : {}),
                ...Css.if(multiline).aifs.oh.mhPx(textAreaMinHeight).$,
              }}
+              // Class name used for the grid table on row hover for highlighting
+              className="textFieldBaseWrapper"


I could envision eventually needing other non-TextFieldBase components controlling the same "I'm hovered, so don't hover the other editable components in the row anymore", so I think it'd potentially be neat to:

Rename ROW_CSS_SELECTOR to BorderHoverParent

Extract textFieldBaseWrapper and name it BorderHoverChild

To have the names align to the general capability we're adding, which is that should a component have borderOnHover=true set, it will hook up its styles to "whatever BorderHoverParent", as long as said BHP doesn't already have a hovered BHC.

## [2.371.0](v2.370.2...v2.371.0) (2024-12-03) ### Features * highlight editable cells on hover ([#1087](#1087)) ([007119b](007119b))

homebound-team-bot · 2024-12-03T20:03:26Z

🎉 This PR is included in version 2.371.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat: editable cells on hover support

4ce98a4

0ces requested review from blambillotte, khalidwilliams, kbesingeryeh and apattersonATX-HB November 18, 2024 17:41

0ces self-assigned this Nov 18, 2024

0ces requested review from stephenh, ksk5280 and chr1sjf0x as code owners November 18, 2024 17:41

fix: default editable value

3f328cc

apattersonATX-HB requested changes Nov 19, 2024

View reviewed changes

fix: PR feedback

c7a280c

0ces requested a review from apattersonATX-HB November 20, 2024 17:48

0ces added 3 commits November 20, 2024 15:56

fix: PR feedback (no editable states)

c3fb6f0

fix: use just CSS for cell highlighting

50b9c12

fix: update CSS for editable cell hover effects

47b5bdc

apattersonATX-HB approved these changes Nov 21, 2024

View reviewed changes

src/components/Table/GridTable.tsx Outdated Show resolved Hide resolved

src/components/Table/components/cell.tsx Outdated Show resolved Hide resolved

apattersonATX-HB reviewed Nov 21, 2024

View reviewed changes

stephenh reviewed Nov 22, 2024

View reviewed changes

bdow reviewed Nov 22, 2024

View reviewed changes

stephenh requested changes Nov 22, 2024

View reviewed changes

fix: use presentation context

bd79761

0ces requested review from stephenh and apattersonATX-HB December 2, 2024 15:34

0ces changed the title ~~feat: editable cells on hover support~~ feat: editable cells bonder on hover Dec 2, 2024

fix: remove console log

2a8c8c6

stephenh reviewed Dec 2, 2024

View reviewed changes

0ces and others added 3 commits December 2, 2024 12:22

Merge branch 'main' into SC-61042/editable-cells-on-hover-support

d92c2b2

fix: PR feedback

fdec08f

fix: clean up PR

3c13d72

stephenh reviewed Dec 2, 2024

View reviewed changes

fix: PR feedback

b3658bf

stephenh reviewed Dec 2, 2024

View reviewed changes

src/components/Table/GridTable.stories.tsx Outdated Show resolved Hide resolved

fix: PR feedback

9be0585

stephenh approved these changes Dec 3, 2024

View reviewed changes

0ces added 2 commits December 3, 2024 11:17

fix: PR feedback

a6eec09

fix: add transition effect to border on hover in TextFieldBase

257e758

0ces changed the title ~~feat: editable cells bonder on hover~~ feat: highlight editable cells on hover Dec 3, 2024

0ces merged commit 007119b into main Dec 3, 2024
7 checks passed

0ces deleted the SC-61042/editable-cells-on-hover-support branch December 3, 2024 20:00

homebound-team-bot pushed a commit that referenced this pull request Dec 3, 2024

chore(release): 2.371.0 [skip ci]

304eab9

## [2.371.0](v2.370.2...v2.371.0) (2024-12-03) ### Features * highlight editable cells on hover ([#1087](#1087)) ([007119b](007119b))

homebound-team-bot added the released label Dec 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: highlight editable cells on hover #1087

feat: highlight editable cells on hover #1087

0ces commented Nov 18, 2024

apattersonATX-HB left a comment

apattersonATX-HB Nov 21, 2024

stephenh Nov 22, 2024

stephenh Nov 22, 2024

stephenh Nov 22, 2024

apattersonATX-HB Nov 25, 2024

stephenh Nov 25, 2024

stephenh Nov 22, 2024

bdow Nov 22, 2024

stephenh Nov 22, 2024 •

edited

Loading

0ces Nov 22, 2024 •

edited

Loading

stephenh Nov 22, 2024

apattersonATX-HB Nov 25, 2024

0ces Nov 25, 2024

stephenh Nov 25, 2024

stephenh left a comment

apattersonATX-HB commented Nov 25, 2024

stephenh Dec 2, 2024

0ces Dec 2, 2024

stephenh Dec 2, 2024

0ces Dec 2, 2024

stephenh left a comment

stephenh Dec 2, 2024

0ces Dec 2, 2024

stephenh Dec 2, 2024

stephenh left a comment

stephenh Dec 3, 2024

homebound-team-bot commented Dec 3, 2024

feat: highlight editable cells on hover #1087

feat: highlight editable cells on hover #1087

Conversation

0ces commented Nov 18, 2024

apattersonATX-HB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenh Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

0ces Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenh left a comment

Choose a reason for hiding this comment

apattersonATX-HB commented Nov 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homebound-team-bot commented Dec 3, 2024

stephenh Nov 22, 2024 •

edited

Loading

0ces Nov 22, 2024 •

edited

Loading