feat(tables): new light/dark mode colors #1833

geotrev · 2024-06-05T23:52:55Z

Description

Adds light / dark mode support for Table components.

Detail

In addition to updating to light/dark compatibility, this PR also:

Uses base styling from IconButton for OverflowButton (with height/width overrides)
Removes isHovered, isFocused, and isActive props from OverflowButton

Checklist

👌 design updates will be Garden Designer approved (add the designer as a reviewer)
🌐 demo is up-to-date (npm start)
⬅️ renders as expected with reversed (RTL) direction
🤘 renders as expected with Bedrock CSS (?bedrock)
💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
♿ tested for WCAG 2.1 AA accessibility compliance
📝 tested in Chrome, Firefox, Safari, and Edge

coveralls · 2024-06-06T00:07:23Z

coverage: 96.083% (+0.04%) from 96.04%
when pulling 575381f on george/table-recolor
into 142286c on next.

coveralls · 2024-06-06T18:35:23Z

coverage: 96.083% (+0.04%) from 96.04%
when pulling 06aa20f on george/table-recolor
into 142286c on next.

ze-flo · 2024-06-06T20:21:25Z

packages/tables/src/styled/StyledRow.ts

+    getColor({
+      variable: 'background.emphasis',
+      transparency: theme.opacity[100],
+      light: { offset: -300 },
+      theme
+    })};


Given the context and how background.subtle is used in other components, wouldn't background.subtle fit better semantically?

Suggested change

getColor({

variable: 'background.emphasis',

transparency: theme.opacity[100],

light: { offset: -300 },

theme

})};

getColor({

variable: 'background.subtle',

transparency: theme.opacity[100],

light: { offset: 300 },

dark: { offset: -600 },

theme

})};

I would be good to understand Design's intent for using a custom color / opacity combo.

With Chrome's color-picker tool, I got the "flattened" HEX value for the stripped row background color. That value corresponds to the color a visual user perceives when the row color (neutral (400:600)/0.08%) is applied over our default light / dark background.

Using this tool, I compared their DeltaE:

Light

custom: #f9f9fa
background.subtle: #f8f9f9

Dark

custom: #1e2328
background.subtle: #1c2227

As you can see, the colors are nearly identical.

Typically, a Delta E of less than 1 is not perceptible to the human eye.

If Table is always rendered on top of a backdrop with a background.default, would it make more sense to reuse background.subtle?

I agree, subtle makes more sense 😄

And thank you for the detailed breakdown / comparison with other components in this use case. TIL!

Something I'm thinking about is what it means for the color and its semantics when we need to do a 6 level offset (plus/minus 600). Does the color respect its design intent by the time its shade is offset by this much? @lucijanblagonic perhaps a question for you.

The backgrounds of both GroupRow and Row (when striped) are set to the below value, per Flo's suggestion, which is the reason for my inquiry/curiosity around color intent with major offsets:

getColor({ variable: 'background.subtle', transparency: theme.opacity[100], light: { offset: 300 }, dark: { offset: -600 }, theme })

That value corresponds to the color a visual user perceives when the row color (neutral (400:600)/0.08%) is applied over our default light / dark background.

that is exactly the intent! we wanted a value that visually matches the v8 group/striped bg treatment. i've attached an image to visually describe the points below:

at first glance, background.subtle was the first logical replacement, but that is intentionally a solid fill, and a solid fill would not respect the bg subtle, raised, and recessed treatments in both light and dark modes.

with that considered we opted for the existing neutral opacity, which targets 700 and 500 @ .8, but in light mode it was too dark of a bg (albeit dark mode bg was just fine)

then opted for the grey-400 / grey-600 @ .08 as it was the closest match.

if there's a more efficient way to obtain that visual parity, then i'm all for it!

packages/tables/src/elements/Row.spec.tsx

jzempel · 2024-06-06T19:38:55Z

packages/tables/src/elements/Row.spec.tsx

+      getColor({
+        variable: 'background.emphasis',
+        transparency: DEFAULT_THEME.opacity[100],
+        light: { offset: -300 },
+        theme: DEFAULT_THEME
+      })


In the context of tests, the preference would be to replace with rgba(PALETTE[color][shade], DEFAULT_THEME.opacity[amount]) rather than repeating getColor code from the component implementation. It just makes the tests slightly more imperative and simpler to maintain.

packages/tables/src/elements/OverflowButton.tsx

packages/tables/src/styled/StyledSortableButton.ts

jzempel · 2024-06-06T19:49:42Z

packages/tables/src/styled/StyledSortableButton.ts

It might be outside the scope of this PR, but when Table.SortableCell is in use, the header cells do not honor table sizing as expected. This bug exists in v8, but it would be nice if we can clean it up in v9.

Is this the bug you're referring to? Where the text/icons aren't vertically centered?

Yes, when you turn Table.SortableCell off, they get placed in their correct positions.

Got it. I'll plan to see if that's addressable in a follow-on PR on main so we can pull it over into next after.

packages/tables/src/styled/StyledHead.ts

jzempel · 2024-06-06T22:44:33Z

packages/tables/src/styled/StyledHeaderRow.ts

+  ${colorStyles}
+
+  ${sizeStyles}


jzempel · 2024-06-06T22:48:41Z

packages/tables/src/styled/StyledRow.ts

@@ -23,36 +23,79 @@ export interface IStyledRowProps {
  size?: ITableProps['size'];
 }

+const baseRowColorStyles = ({ theme, isStriped }: IStyledRowProps & ThemeProps<DefaultTheme>) => {
+  return css`
+    border-bottom: ${theme.borders.sm} ${getColor({ variable: 'border.subtle', theme })};


[nit] the preference is usually to separate border-bottom from border-bottom-color. And moving these values out to local variables makes the CSS more readable.

jzempel · 2024-06-06T22:49:59Z

packages/tables/src/styled/StyledRow.ts

+    })};
+  `;
+};
+
 export const StyledBaseRow = styled.tr<IStyledRowProps>`


Fwiw, this combined Styled... components in one file is a bad code smell. Might be good to clean it up here.

lucijanblagonic · 2024-06-07T09:10:32Z

There is an inconsistency with border-bottom-color on Hover. Our designs specify border bottom color to be opacity/primary/200

Current implementation

Updated example

Not sure if there are any other implications to the border-color, @steue please take a look as well.

jzempel

The dependency for @zendeskgarden/react-buttons needs to be added to table's package.json

jzempel

Looks like Table.Row isSelected styling got partially lost. Perhaps due to a $ prop spread? 🤷

jzempel · 2024-06-11T14:07:53Z

packages/tables/src/styled/StyledCell.ts

@@ -16,7 +16,7 @@ const COMPONENT_ID = 'tables.cell';

 export interface IStyledCellProps
  extends Pick<ICellProps, 'isMinimum' | 'isTruncated' | 'hasOverflow' | 'width'> {
-  size?: ITableProps['size'];
+  $size?: ITableProps['size'];


Please note that the picked props above will continue to leak in styled-components v6.

I went ahead and converted the remaining props to transient props and I think captured all the remaining spreadable props as explicitly converted. Doing another self review now, as well.

jzempel · 2024-06-11T14:20:50Z

packages/tables/src/elements/Row.tsx

        ref={ref}
+        tabIndex={isReadOnly ? undefined : -1}


I like the element component move, but in order to match previous the tabIndex should follow {...otherProps} so that it wins.

jzempel · 2024-06-11T14:33:42Z

packages/tables/src/styled/StyledHeaderRow.ts

+  ${colorStyles}
+
+  ${sizeStyles}


Suggested change

${colorStyles}

${sizeStyles}

${sizeStyles}

${colorStyles}

[nit] per API docs, these should be switched .. although it shouldn't happen, color should hold the more significant position if there happens to be a cascade.

jzempel · 2024-06-11T14:37:04Z

packages/tables/src/styled/StyledRow.ts

+  `;
+};
+
+export const StyledRow = styled(StyledBaseRow).attrs<IStyledRowProps>({


Suggested change

export const StyledRow = styled(StyledBaseRow).attrs<IStyledRowProps>({

export const StyledRow = styled(StyledBaseRow).attrs({

[nit] type should no longer be needed for the attrs.

geotrev · 2024-06-11T16:47:31Z

@jzempel think you found a limitation with how backgrounds/borders intermingle in cases of truncation re: isSelected. Will wait for design to weigh in (left a comment in the design inventory).

jzempel

🎉

steue

the accommodations look good in light mode! but the borders look too dark when isSelected is set to true. it looks like it is set to blue-900? we have it set to blue-500 @ 16%. was this the border issue you were speaking of?

i was able to target it directly in inspect to get the desired effect.

geotrev · 2024-06-13T14:04:52Z

@steue In order to resolve the issue JZ reported above, we need a solid border as one with opacity will result in truncated rows hiding the border. 😅

steue · 2024-06-14T00:43:25Z

😓 in light mode isSelected treatment looks good. but it still looks too dark than intended in dark mode. since we can not use opacity, could we use a bespoke value to achieve the same effect? wdyt @lucijanblagonic ?

lucijanblagonic · 2024-06-17T09:33:41Z

It's probably best to use colors that we have available within those 12 steps, and accept the technical limitations. A big change in saturation and lightness in dark mode (like using blue-800/700) doesn't look good, and it's OK to have a subtle border as the background is much more prominent.

consistent color for border-bottom-color in both states (selected, selected + hover)
use blue-300 for light mode
use blue-900 for dark mode

With changes applied in inspector

Screenshot of 300/900 border color

steue

i think this is an acceptable workaround for the limitation posed. thanks for the discussion @lucijanblagonic! sorry and thank you @geotrev for explaining the issue and patience with the back and forth!

geotrev added the PR: New Feature 🚀 label Jun 5, 2024

geotrev self-assigned this Jun 5, 2024

geotrev requested a review from a team as a code owner June 5, 2024 23:52

zendesk-garden deployed to staging June 6, 2024 00:08 View deployment

zendesk-garden deployed to staging June 6, 2024 18:36 View deployment

ze-flo reviewed Jun 6, 2024

View reviewed changes

packages/tables/src/elements/Row.spec.tsx Outdated Show resolved Hide resolved

jzempel requested changes Jun 6, 2024

View reviewed changes

jzempel requested changes Jun 7, 2024

View reviewed changes

geotrev added 8 commits June 10, 2024 09:35

feat(tables): adds light/dark mode colors

4ff9f21

chore: updates Row and Table

2d7af5f

chore: adds light/dark to sortable button

a83b144

chore(tables): updates tests

ff883f7

chore: pre-review updates

f2dfd96

chore: updates migration.md

230adbb

chore: code & design review

c952043

fix: tests

f17a9d5

geotrev force-pushed the george/table-recolor branch from 06aa20f to f17a9d5 Compare June 10, 2024 18:34

fix: transient prop consistency

58842be

zendesk-garden deployed to staging June 10, 2024 19:57 View deployment

geotrev added 2 commits June 10, 2024 15:12

fix: sort icon colors to foreground.subtle

3ff0cc3

fix: use background.subtle on grouprow background

508e1de

zendesk-garden deployed to staging June 10, 2024 20:46 View deployment

geotrev requested review from jzempel, steue and ze-flo June 11, 2024 14:08

fix: correct colors on sortable icon when inactive

03bec36

zendesk-garden deployed to staging June 11, 2024 14:38 View deployment

jzempel requested changes Jun 11, 2024

View reviewed changes

chore: review changes

d91d990

zendesk-garden deployed to staging June 11, 2024 17:14 View deployment

fix: better prop management

34a103f

zendesk-garden deployed to staging June 12, 2024 15:27 View deployment

fix: use solid colors for table row borders to prevent clipping

36ef8bb

zendesk-garden deployed to staging June 12, 2024 18:48 View deployment

jzempel approved these changes Jun 12, 2024

View reviewed changes

steue reviewed Jun 12, 2024

View reviewed changes

fix(tables): use brighter colors for table row borders

6185172

zendesk-garden deployed to staging June 13, 2024 15:12 View deployment

chore: use new solid colors for borders

5457e16

geotrev requested a review from steue June 17, 2024 15:56

zendesk-garden deployed to staging June 17, 2024 16:14 View deployment

steue approved these changes Jun 18, 2024

View reviewed changes

geotrev merged commit 54514f3 into next Jun 18, 2024
5 checks passed

geotrev deleted the george/table-recolor branch June 18, 2024 22:15

ze-flo mentioned this pull request Oct 3, 2024

fix(tables): replace invalid <div> from Table OverflowButton w/ <span> #1939

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tables): new light/dark mode colors #1833

feat(tables): new light/dark mode colors #1833

geotrev commented Jun 5, 2024 •

edited

Loading

coveralls commented Jun 6, 2024

coveralls commented Jun 6, 2024

ze-flo Jun 6, 2024

ze-flo Jun 6, 2024 •

edited

Loading

geotrev Jun 7, 2024

geotrev Jun 7, 2024

geotrev Jun 10, 2024

geotrev Jun 10, 2024

steue Jun 11, 2024

jzempel Jun 6, 2024

jzempel Jun 6, 2024

geotrev Jun 10, 2024 •

edited

Loading

jzempel Jun 10, 2024

geotrev Jun 10, 2024

jzempel Jun 6, 2024

jzempel Jun 6, 2024

jzempel Jun 6, 2024

lucijanblagonic commented Jun 7, 2024

jzempel left a comment

jzempel left a comment

jzempel Jun 11, 2024

geotrev Jun 11, 2024

jzempel Jun 11, 2024

jzempel Jun 11, 2024

jzempel Jun 11, 2024

geotrev commented Jun 11, 2024

jzempel left a comment

steue left a comment

geotrev commented Jun 13, 2024 •

edited

Loading

steue commented Jun 14, 2024 •

edited

Loading

lucijanblagonic commented Jun 17, 2024 •

edited

Loading

steue left a comment •

edited

Loading

	export const StyledRow = styled(StyledBaseRow).attrs<IStyledRowProps>({
	export const StyledRow = styled(StyledBaseRow).attrs({

feat(tables): new light/dark mode colors #1833

feat(tables): new light/dark mode colors #1833

Conversation

geotrev commented Jun 5, 2024 • edited Loading

Description

Detail

Checklist

coveralls commented Jun 6, 2024

coveralls commented Jun 6, 2024

Choose a reason for hiding this comment

ze-flo Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Light

Dark

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

geotrev Jun 10, 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

Choose a reason for hiding this comment

lucijanblagonic commented Jun 7, 2024

jzempel left a comment

Choose a reason for hiding this comment

jzempel 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

geotrev commented Jun 11, 2024

jzempel left a comment

Choose a reason for hiding this comment

steue left a comment

Choose a reason for hiding this comment

geotrev commented Jun 13, 2024 • edited Loading

steue commented Jun 14, 2024 • edited Loading

lucijanblagonic commented Jun 17, 2024 • edited Loading

steue left a comment • edited Loading

Choose a reason for hiding this comment

geotrev commented Jun 5, 2024 •

edited

Loading

ze-flo Jun 6, 2024 •

edited

Loading

geotrev Jun 10, 2024 •

edited

Loading

geotrev commented Jun 13, 2024 •

edited

Loading

steue commented Jun 14, 2024 •

edited

Loading

lucijanblagonic commented Jun 17, 2024 •

edited

Loading

steue left a comment •

edited

Loading