eui icon indicating copy to clipboard operation
eui copied to clipboard

Published 20 hours ago • verified publisher icon elastic

[Visual Refresh] Make `EuiDataGrid` and `EuiTable` hover consistent

Open acstll opened this issue 5 months ago • 7 comments

Summary

Closes https://github.com/elastic/eui-private/issues/301

[!IMPORTANT] Separate PR to update border-strong-* token values in Amsterdam ~~is still underway~~ ~~in draft mode~~ ready!

Following the design spec, this PR introduces changes to

  • background colors mainly for hover, including selected states, for both EuiTable and EuiDataGrid
  • colors for cell selection outline in EuiDataGrid

The update adds a new colors.backgroundBaseInteractiveSelectHover token, to differentiate the hover state for selected rows. And removes hover states completely for the "stripes" version of Data Grid.

Demo videos (Borealis/light only)

Data Grid

https://github.com/user-attachments/assets/f10a0fef-c1a7-4361-bab9-82d1b9e21809

Table

https://github.com/user-attachments/assets/3e787519-aa10-426b-a841-936d9549179b

Amsterdam

Styles for the Amsterdam theme were left untouched. There is only one small change we can consider a fix: for EuiTable the hover state color when there's an onClick handler applies (it didn't).

~~I have a few doubts regarding the adaptation for the legacy Amsterdam theme:~~

  • ~~No hover color for stripes=true seems too breaking. I would keep it. @ek-so what do you think? (@mgadewoll I implemented this in the component code, not CSS; is there an accepted way to check if the theme is Amsterdam?)~~
  • ~~I think the new backgroundBaseInteractiveSelectHover color is too different from backgroundBaseInteractiveSelect 🤔~~
    • ~~and the values for SCSS are not exactly matching~~
  • ~~The value for borderStrongPrimary was updated to keep the selected outline "primary" blue (this should be safe, but I felt like I should mention it) — @ek-so you think this is also OK?~~

QA

  • [ ] check colors and logic match the spec in:
    • [ ] Borealis (light)
    • [ ] Borealis (dark)
  • [ ] check styles remain the same:
    • [ ] Amsterdam (light)
    • [ ] Amsterdam (dark)
  • [ ] would new visual regression tests be needed/beneficial?

General checklist

  • Browser QA
    • [x] Checked in both light and dark modes
    • [x] Checked in both MacOS and Windows high contrast modes
    • [x] Checked in mobile
    • [x] Checked in Chrome, Safari, Edge, and Firefox
    • [ ] ~~Checked for accessibility including keyboard-only and screenreader modes~~
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • [x] A changelog entry exists and is marked appropriately.
    • [ ] ~~If applicable, added the breaking change issue label (and filled out the breaking change checklist)~~
  • Designer checklist
    • [ ] ~~If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)~~

acstll avatar Jun 09 '25 11:06 acstll

Converting to draft because a cypress test is failing — ~~seems to be failing not because of the colors of the cell outline changed, but something else?~~

acstll avatar Jun 10 '25 06:06 acstll

@acstll I don't think the dataGrid failing test is actually related to the changes. I've noticed those tests failing before locally 🫠 Here's a small PR to "fix" the issue.

mgadewoll avatar Jun 10 '25 18:06 mgadewoll

Hey @acstll, I'm afraid I need some clarification on the questions. Are they related only to Amsterdam? Why then we change anything there? From what I see, in Borealis it all looks good an as expected.

ek-so avatar Jun 11 '25 08:06 ek-so

Hey @acstll, I'm afraid I need some clarification on the questions. Are they related only to Amsterdam? Why then we change anything there?

@ek-so it's all clear now! We shouldn't change anything visually on Amsterdam; this was not clear to me and all related questions come from that confusion, so sorry — also the question regarding borderStrongPrimary is mainly related to the implementation (and has been cleared with Lene already) 🙂

From what I see, in Borealis it all looks good an as expected.

thanks for checking 🙏

acstll avatar Jun 11 '25 09:06 acstll

Quick status update: I changed the implementation to use (new) component tokens instead of that bit of TS logic, in order to have Amsterdam remain exactly the same.

Will do the following next:

  • [x] ensure (color) token values are correct across formats (both Borealis and Amsterdam)
  • [x] double-check docs, Storybook, VRT
  • [x] check cypress failing again (even after rebasing with the fix) — will do this last
    • this time it was actually about the focus color (borderStrongPrimary)
  • [x] (and that extra PR to update border tokens in Amsterdam) #8794

acstll avatar Jun 12 '25 06:06 acstll

:green_heart: Build Succeeded

History

  • :green_heart: Build #564 succeeded 94c55b0239bef64e1bfe4b5f4665e8f3e8748022
  • :green_heart: Build #512 succeeded e4f5d041ae8879bd8864766c24164483264c5904
  • :broken_heart: Build #511 failed 828355aacddb42016c98133c7ce4f66ed1c3aa06
  • :broken_heart: Build #510 failed 8cd38f51b0331fccb2c619452c4361200550cf52
  • :green_heart: Build #500 succeeded 10e62b6bb92ef4691f7590b5574a5fc523acc12e
  • :green_heart: Build #497 succeeded 0ebdcd5c99d22eb9e5098cb6a05791c7c58f496e

cc @acstll

elasticmachine avatar Jun 18 '25 16:06 elasticmachine

:green_heart: Build Succeeded

History

  • :broken_heart: Build #4222 failed 94c55b0239bef64e1bfe4b5f4665e8f3e8748022
  • :green_heart: Build #4178 succeeded e4f5d041ae8879bd8864766c24164483264c5904
  • :broken_heart: Build #4177 failed 828355aacddb42016c98133c7ce4f66ed1c3aa06
  • :green_heart: Build #4168 succeeded 10e62b6bb92ef4691f7590b5574a5fc523acc12e
  • :broken_heart: Build #4166 failed 0ebdcd5c99d22eb9e5098cb6a05791c7c58f496e
  • :broken_heart: Build #4135 failed 7e58e9a9fc269c8e6a926c910f8b0e780908d047

cc @acstll

elasticmachine avatar Jun 18 '25 17:06 elasticmachine

@acstll after this is merged, I would inform the discover team about it additionally, because they have a custom implementation in discover for this thing, and it doesn't look clean currently. We promised to do that as far as I recall, here. /cc @JasonStoltz @l-suarez

ek-so avatar Jun 19 '25 06:06 ek-so

@acstll after this is merged, I would inform the discover team about it additionally, because they have a custom implementation in discover for this thing, and it doesn't look clean currently. We promised to do that as far as I recall, here.

@ek-so I created a sub-task to track it (https://github.com/elastic/eui-private/issues/360), we'll be glad to help

acstll avatar Jun 19 '25 08:06 acstll