-
Notifications
You must be signed in to change notification settings - Fork 841
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
Fix table re-rendering by showing/hiding actions column via CSS instead #665
Conversation
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.
Code LGTM but I haven't tested in browser. I'm going to add @nreese as a reviewer to verify this fixes his performance issue.
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.
Seems to work for me, just one comment about the CSS.
src/components/table/_table.scss
Outdated
@@ -144,3 +144,14 @@ | |||
overflow: visible; /* 1 */ | |||
} | |||
} | |||
|
|||
.euiTableCellContent--showOnHover { | |||
> * { |
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.
Makes way more sense to handle the hover opacities via css. But, universal selectors are not great in practice and we're trying to stay away from them in EUI as it can have unintended behaviors if the DOM is not exactly as the CSS is expecting.
To remedy this, I think you need to pass the showOnHover
prop down to the child element of the EuiTableRowCell and apply the opacities directly to this. This will ensure you're targeting the correct element to show/hide.
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.
lgtm
Thanks for fixing this.
@cchaos Thanks for the feedback - updated PR to replace the wildcard selector with |
src/components/table/_table.scss
Outdated
@@ -146,7 +146,7 @@ | |||
} | |||
|
|||
.euiTableCellContent--showOnHover { | |||
> * { | |||
.euiTableCellContent__hoverItem { | |||
opacity: 0; | |||
|
|||
.euiTableRow:hover > .euiTableRowCell > &, |
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.
The specificity of this selector is too high. Remove the > .euiTableRowCell >
and it should still work with just .euiTableRow:hover &
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.
Updated!
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.
Thanks Jen!
Fixes #441
This PR fixes unnecessary re-rendering of the entire
EuiBasicTable
component upon hovering over table rows by:hoverRow
state onEuiBasicTable
.euiTableCellContent--showOnHover
toEuiTableRowCell
when propshowOnHover
is trueThe
.euiTableCellContent--showOnHover
class has styling rules to hide its content by default and show it when its parent.euiTableRow
is hovered.Normal/keyboard navigation for single and multiple action column remains the same (no functionality changes).