Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[EuiDataGrid] Provide row elements to wrap cells #5285
[EuiDataGrid] Provide row elements to wrap cells #5285
Changes from 39 commits
3de3ce9
4594779
04d3ab5
9a6dc07
f786f5f
06ac499
0129ea4
c05b457
179da4a
24f5c53
a2ccb72
eb4344c
535a2a9
9151350
814feb1
95b7008
0ba3a02
1569aa0
9a6df52
ae336f9
8dc8ad4
7633c1b
c3e8b49
5ac481f
e0dd86e
ca721f2
86af5ba
2553d89
fdc387e
d468c85
dea1eb9
c995387
a60d734
ae515b6
f5b02ef
82614cf
8336957
baa7b25
937403e
705edad
55a5f95
420b37d
ad5c72b
6d10f54
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not totally clear on what this
dataItemsRendered
or thedata-itemsrendered
attribute is being used for - is it just for Cypress testing? TBH, I'd almost rather do visual snapshot testing over this approach. 1) it feels like it'd be more intuitive to compare outputs & state, and 2) it wouldn't impact realtime performanceThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was to provide a way for the test to determine when the rendering is stable. The hooks + virtualization means the shell of the grid + header row is rendered at the initial pass, but then a second (maybe even a third) pass is needed to draw the cells. I couldn't think of a non-hacky way to do this and everything in Cypress's docs + googling said there should be some signal in the DOM to wait for, so I added the data attribute with the virtualization state to hook into.
Totally open to other ideas!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh gotcha, sorry! I was totally off on what this was doing in that case, apologies (I thought it was a way of determining cell content 🙈)
I think my worry is potential impact for production grids with lots of data / if this bit of code is overengineered for its purpose. Alternatives:
onItemsRendered
gets called instead?data-test-subj
on our individual cells and check that the expected number of cells exists instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was anticipating this full information would be useful in future tests against virtualization. Scroll & verify the row&column indices, set the
overscanCount
prop and verify, etc.Maybe, to handle the perf impacts as this does update the attribute quite aggressively (though I didn't notice any lag when testing), we enable this with a
debug
flag on the component? This could open/enable other console logs or other ways of exposing info around the inner workings.A true/false flag may work, but some tests may need that flag to toggle back to false until reaching stability again.
I can't find a (non-horrible hacky) way to have Cypress wait for a number of elements. We can select for
[role=gridcell]
, but from what I can tell trying to wait for N of them is an anti-pattern. We could add adata-test-subj
with${row},${col}
and select/wait for that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oo, I like this option a lot! A debug/cypress flag would be fine, but between the two I think the
row,col
data-test-subj is elegant and could be helpful in general / potentially even in prod debugging.Ha I have yolo'd too many Cypress anti-patterns in my day attempting to wait for the flakily-un-waitable (recursive xhr polls waiting server side asynchronous cron jobs probably being my least favorite)... 🙈 I definitely appreciate the dialogue and compromise and think we have a few solid approaches here (for the future as well)!