eui icon indicating copy to clipboard operation
eui copied to clipboard

[EuiInMemoryTable] Use the `.euiTableRow-isSelectable` className to indicate state

Open drewdaemon opened this issue 1 year ago • 3 comments

Describe the bug The EuiInMemoryTable assigns the euiTableRow-isSelectable class to non-selectable rows.

Impact and severity The impact is largely felt when writing automated tests. For example, this routine in Kibana's functional test code is no longer accurate since isSelectable is being applied to all rows. I don't think this is high impact since the test can always examine the checkboxes themselves.

Environment and versions

  • EUI version: v93.0.0
  • React version: 17.0.2

To Reproduce Steps to reproduce the behavior:

  1. Go to https://eui.elastic.co/#/tabular-content/in-memory-tables#in-memory-table-selection
  2. Load the rows in the example
  3. Inspect the dom

Expected behavior I would expect isSelectable CSS class to only be applied to rows for which selection.selectable returned true 🤷‍♂️

drewdaemon avatar Feb 06 '24 18:02 drewdaemon

I definitely see what you're seeing, but just to clarify, are you saying this was the case and broke at some point in EUI history? From what I'm seeing in the source code, it looks like this always the way EuiInMemoryTable and EuiBasicTable has behaved.

https://github.com/elastic/eui/blob/e8407ff52232e70b029f37e36e0b2cc25b82d493/src/components/table/table_row.tsx#L21-L25

helps shed some light on this - it looks like the .euiTableRow-isSelectable class is really mostly there for mobile styling and isn't intended to indicate state. So it's just named somewhat poorly 🥲

We can definitely consider revisiting this during EuiTable's Emotion conversion - we won't need the className for CSS at that point and can re-appropriate it as a state indicator.

cee-chen avatar Feb 06 '24 18:02 cee-chen

@cee-chen interesting. I had assumed that the class was once applied only to selectable rows because of that code I saw in our FTs. But, it may well be that the author of that code made an incorrect assumption.

We can definitely consider revisiting this during EuiTable's Emotion conversion - we won't need the className for CSS at that point and can re-appropriate it as a state indicator.

This could be nice! But, as I've thought about it more, you could also argue that internal EUI classes aren't really part of your supported contract. I'll leave it up to you

drewdaemon avatar Feb 07 '24 14:02 drewdaemon

I'm definitely open to doing it during the Emotion conversion! We use other -isX modifiers/CSS classes in EUI across the codebase and while it's not a guaranteed contract, it's definitely very nice to have!

cee-chen avatar Feb 07 '24 17:02 cee-chen

Addressed by https://github.com/elastic/eui/pull/7632. Will be merged into main once the feature branch with all EuiTable Emotion conversions is opened, so should reach Kibana main in another 2-3 weeks.

cee-chen avatar Apr 01 '24 16:04 cee-chen