[EuiDataGrid] Various footer fixes #5720
Conversation
cee-chen
force-pushed
the
datagrid/footer-fixes
branch
from
7ff929d to
a3fe7fd
Compare
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5720/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5720/ |
| // Turn off the cell expansion button if the footer cell is empty | ||
| if (!value) setCellProps({ isExpandable: false }); |
There was a problem hiding this comment.
Can you document this somewhere in the example text? Something as simple as:
When rendering footer cell values, we encourage turning off cell expansion on cells without content with
setCellProps({ isExpandable: false }).
There was a problem hiding this comment.
To be honest, this really feels like the DataGrid should know if any cells are empty, to turn off cell expansion by default. But I'll create a specific issue.
There was a problem hiding this comment.
The data grid inherently has no concept of the actual underlying data that's being rendered, and it would be non-trivial to change that architectural direction. I know there are a few open issues about that behavior however, e.g. #4108. @chandlerprall do you mind expanding on this?
There was a problem hiding this comment.
Apart from being able to detect if they're empty (I do have some thoughts there), I don't know if we can define empty in a meaningful way. I can imagine a waffle chart (and maybe ML has one of theirs implemented with the data grid?) where the only "content" is color, but expanding the cell would provide additional information.
There was a problem hiding this comment.
Can it not know if the popoverContents is empty?
There was a problem hiding this comment.
No - popover contents now use the same paradigm as renderCellValue, we ask for a render function and are unopinionated about what's in it. Additionally, we don't render the cell popover until after the expand button has been clicked so we can't easily inspect the contents before determining whether the button should even render. Pre-rendering popovers to inspect contents feels like an anti-pattern to me and could have a negative performance impact.
I also think it's worth hashing out what we define as "empty". We can examine the cell content ref's innerHtml, but what happens when the consumer passes a space or empty <p> or <br> tag? We can examine the cell content ref's innerText, but what happens when the consumer has passed an image, chart, or some other media without text nodes?
In the end what I think this boils down is that the consumer knows their content better than we do, and while we can try to enforce things rigidly, ultimately nothing is foolproof and providing guidelines is sometimes the best we can do. At a quick glance it also looks like there's only 2 tables in Kibana currently using footer rows, so it should be fairly quick to do an audit of them if we needed to.
There was a problem hiding this comment.
This. This is "empty" 🤣
But I get it, the answer right now is "we can't", but I'd strongly suggest figuring out if we "can" somehow, at some point. We all know consumers who use plug & play components do not think twice after just handing down their data. They just think "That's how its supposed to be because that's how the component is built"
…cell Using a cell `key` with a column ID causes the cell node reference to move, instead of the cell/node reference staying in place and cell props changing (which is how react-window behaves)
cee-chen
force-pushed
the
datagrid/footer-fixes
branch
from
1763ad2 to
e72ef0a
Compare
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5720/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5720/ |
|
@cchaos Just to check, do you still have any more change requests or do this look good to merge to you? |
Summary
There's 1 main bugfix in here and 2 documentation fixes, so I recommend following along by commit.
Footer focus bug:
Repro steps:
amountcolumn to the left or rightamountcolumn's footer cellFooter cell
@cchaos noted in #5710 that it felt odd for empty cell footers to have an expansion button.
Before:
After:
This isn't something we can enforce at the EUI level, but we can add this it as an example/recommendation for consuming applications with footers to use.
Checklist