search

jpmorganchase / salt-ds

React UI components built with a focus on accessibility, customization and ease-of-use
https://www.saltdesignsystem.com
Apache License 2.0
131 stars 89 forks source link

Ag grid theme copy cell background color is flipped #3922

Closed origami-z closed 1 month ago

origami-z commented 3 months ago

Package name(s)

AG Grid Theme (@salt-ds/ag-grid-theme)

Package version(s)

v2.0.1

Description

When copy cell in dark mode, background flips to white (the blink), was not the case before v2.

In light mode, it's flipped to dark.

Steps to reproduce

  1. Go to https://www.saltdesignsystem.com/salt/components/ag-grid-theme/examples#variants
  2. Set to dark mode
  3. Copy any cell (ctrl + c)
  4. Observe the blinking color is white

Expected behavior

Possible solution: --ag-range-selection-highlight-color: var(--salt-overlayable-rangeSelection);, or create a new token to be more prominent

https://www.ag-grid.com/react-data-grid/global-style-customisation-selections/#cell-selections

Operating system

Browser

Are you a JPMorgan Chase & Co. employee?

origami-z commented 2 months ago

Cortado Goal: To be fixed and released

origami-z commented 2 months ago

Aug 12 - found potential color, @origami-z to work with @dplsek

origami-z commented 2 months ago

Prototype stackblitz - https://stackblitz.com/edit/salt-ag-grid-v31-theme-v2-lkzmse?file=style.css,App.jsx see later comment

joshwooding commented 2 months ago

Aug 20 - Work to resume after button work is done

origami-z commented 2 months ago

New prototypes

dplsek commented 2 months ago

We will be using blue 500/50 in Salt CURRENT and blue/teal 700/300 in Salt NEXT for the flashing copy state. Links included above to the coded examples and the corresponding Figma file with contrast ratios for text can be found here

origami-z commented 2 months ago

We need a set of tokens in theme, shall we call it --salt-selectable-background-highlight (more or less align with ag grid's variable --ag-range-selection-highlight-color)? Potential mapping below, @pseys to confirm

Salt current

Characteristics Palette Foundation
--salt-selectable-background-highlight --salt-palette-interact-background-highlight (light) --salt-color-blue-50
--salt-selectable-background-highlight --salt-palette-interact-background-highlight (dark) --salt-color-blue-500

Salt next

Characteristics Palette Foundation
--salt-selectable-background-highlight --salt-palette-accent-action-highlight (light) --salt-color-blue-300 or --salt-color-teal-300
--salt-selectable-background-highlight --salt-palette-accent-action-highlight (dark) --salt-color-blue-700 or --salt-color-teal-700
origami-z commented 2 months ago

Extend a sprint for token review

mark-tate commented 2 months ago

Espresso @pseys to review and released by 13th September

pseys commented 1 month ago

We need a set of tokens in theme, shall we call it --salt-selectable-background-highlight (more or less align with ag grid's variable --ag-range-selection-highlight-color)? Potential mapping below, @pseys to confirm

Salt current

Characteristics Palette Foundation --salt-selectable-background-highlight --salt-palette-interact-background-highlight (light) --salt-color-blue-50 --salt-selectable-background-highlight --salt-palette-interact-background-highlight (dark) --salt-color-blue-500

Salt next

Characteristics Palette Foundation --salt-selectable-background-highlight --salt-palette-accent-action-highlight (light) --salt-color-blue-300 or --salt-color-teal-300 --salt-selectable-background-highlight --salt-palette-accent-action-highlight (dark) --salt-color-blue-700 or --salt-color-teal-700

@origami-z @dplsek I'm happy with this proposal. If we're looking to release this update on 13 September let me know and I'll make sure the Figma style libraries are updated as well.

origami-z commented 1 month ago

@pseys yes, if we can make this change in this release, it would be good. Button release remains the highest priority across the board

origami-z commented 1 month ago

New proposal to use the same token from range selection token, which works mostly ok except when cell ranges were already selected, then copy would result the same color highlight. Updated prototypes to show this.

pseys commented 1 month ago

FYI @origami-z @dplsek

Characteristic Theme Palette Foundation Foundation (Fade)
--salt-overlayable-background-highlight Next --salt-palette-alpha --salt-color-black-30a
--salt-color-white-30a
Legacy --salt-palette-neutral-highlight --salt-color-black-fade-background-highlight --salt-color-black + --salt-opacity-25
--salt-color-white-fade-background-highlight --salt-color-white + --salt-opacity-25
bhoppers2008 commented 1 month ago

@pseys to document the change and update light/dark modes and the Salt (Next) Styles to keep alignment with code.

mark-tate commented 1 month ago

Frappe Goal: released by EOS