Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
drewdaemon opened this issue Feb 6, 2024 · 4 comments

Comments

@drewdaemon
Copy link

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 drewdaemon added bug ⚠️ needs validation For bugs that need confirmation as to whether they're reproducible labels Feb 6, 2024
@cee-chen
Copy link
Contributor

cee-chen commented Feb 6, 2024

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.

/**
* Indicates if the table has a single column of checkboxes for selecting
* rows (affects mobile only)
*/
isSelectable?: boolean;

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.

@drewdaemon
Copy link
Author

@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

@cee-chen
Copy link
Contributor

cee-chen commented Feb 7, 2024

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 cee-chen added feature request emotion and removed bug ⚠️ needs validation For bugs that need confirmation as to whether they're reproducible labels Feb 7, 2024
@cee-chen cee-chen changed the title EuiInMemoryTable assigns the euiTableRow-isSelectable class to non-selectable rows [EuiInMemoryTable] Use the .euiTableRow-isSelectable className to indicate state Feb 7, 2024
@cee-chen cee-chen self-assigned this Mar 28, 2024
@cee-chen
Copy link
Contributor

cee-chen commented Apr 1, 2024

Addressed by #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 cee-chen closed this as completed Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants