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

[EuiDataGrid] Actually fix calls to tabbable instead of moving the issue to a different part of the code #5163

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Sep 9, 2021

Summary

Fixes #5162

Um, so. I omitted a ! in an if statement in #5136 which interacted with itself in an extreme way when a column resize causes a column to drop off into the virtualization void.

This PR:

  • adds an optimization to only process a cell once even if there were multiple mutations (not the crux of the issue, but one flame graph implicated this)
  • actually finds the cell's div to pass to tabbable, instead of flagging the entire grid

Testing:

Set the main grid demo's page size to 100, then resize a column to drop one or more off the end.

/cc @kqualters-elastic if you'd like to test this within your grid

Before

flame graph showing really bad performance

After

flame graph showing significantly better performance

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile

  • Checked in Chrome, Safari, Edge, and Firefox
    - [ ] Props have proper autodocs and playground toggles
    - [ ] Added documentation
    - [ ] Checked Code Sandbox works for any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@chandlerprall chandlerprall changed the title [EuiDataGrid] Actually fix calls to tabbable instead of move the issue to a different part of the code [EuiDataGrid] Actually fix calls to tabbable instead of moving the issue to a different part of the code Sep 9, 2021
@@ -248,16 +248,16 @@ const IS_JEST_ENVIRONMENT = global.hasOwnProperty('_isJest');
* and search its ancestors for a div[data-datagrid-cellcontent], if any,
* which is a valid target for disabling tabbing within
*/
function getParentCellContent(_element: Node | HTMLElement) {
export function getParentCellContent(_element: Node | HTMLElement) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export for testing

element.hasAttribute('data-datagrid-cellcontent')
element && // we haven't walked off the document yet
element.nodeName !== 'div' && // looking for a div
!element.hasAttribute('data-datagrid-cellcontent') // that has data-datagrid-cellcontent
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this ! is the main change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♀️ I feel super bad for not noticing this/digging into it a bit more deeply in the first PR. Apologies!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5163/

@chandlerprall chandlerprall marked this pull request as ready for review September 10, 2021 16:15
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5163/

Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it!! Thanks for the test changes - it's even more helpful for me to read the test cases in your own words!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5163/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants