[EuiTable EuiBasicTable] Fix nested content alignment when selection is enabled#7895
Conversation
- moves conditional padding to the outer wrapper from the cell to align nested table content with the parent table column if a selection is present and prevent issues with overlapping content
- ensures correct positioning by appling styles only to direct children
- updates EuiTableRowCell stories to ensure expected controls behavior
|
Thanks for looking into this! I noticed in the "after" screenshot, is it intentional that we keep that margin for the whole expanded row? 
Here's an older screenshot of one of our expanded rows where you can see there's no left padding:
|
@walterra Yes, that's expected and was already the existing behavior but ONLY when selection is enabled for the table. If enabled we offset the nested table by the selection checkbox width. Here how it looks without selection enabled: |
|
Oh great, thanks for confirming! I didn't notice I picked a screenshot that old that didn't have the bulk actions yet 😅 . |
| // Offset expanded & selectable rows by the checkbox width to line up content with the 2nd column | ||
| // Set on the `<td>` because padding can't be applied to `<tr>` elements directly | ||
| checkboxOffset: css` | ||
| .euiTableRowCell:first-child { |
There was a problem hiding this comment.
I'm probably being a little nit-picky here, but wouldn't changing this to & > .euiTableRowCell:first-child { also have fixed the issue? I'm a little hesitant to apply new props to EuiTableRowCell mostly because it's a public top-level API change 🤷 (which would mean breaking changes or deprecations if they ever change again in the future)
There was a problem hiding this comment.
Oh damn, yes you're absolutely right! I should have noticed that 🤦♀️
That is definitely a more straight forward solution. I'll update 👍
There was a problem hiding this comment.
Reverted the API changes here in favor of the suggested style solution.
💚 Build Succeeded
History
|
cee-chen
left a comment
There was a problem hiding this comment.
Beautiful!! Thank you for the story updates as well Lene!! ❤️
|
Going to go ahead and merge this for this week's release |
`v95.3.0` ⏩ `v95.4.0` _[Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_ --- ## [`v95.4.0`](https://github.com/elastic/eui/releases/v95.4.0) - Added `anomalyChart`, `anomalySwimLane`, `changePointDetection`, `fieldStatistics`, `logPatternAnalysis`, `logRateAnalysis` and `singleMetricViewer` glyph to `EuiIcon` ([#7873](elastic/eui#7873)) **Bug fixes** - Fixed overlapping content in `EuiBasicTable` for expanded and selectable table rows ([#7895](elastic/eui#7895)) - Fixed the alignment of `EuiBasicTable` mobile actions ([#7895](elastic/eui#7895)) **Accessibility** - Improved `EuiStat`'s screen reader accessibility ([#7864](elastic/eui#7864)) --- ## Additional Changes - reverts temporary fix for overlapping content in nested tables done in PR [#188374](#188374) --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Summary
closes #7888
Changes
Previously the implementation of applying the padding on each nested table row resulted in overlapping content issues. This PR proposes to apply the padding on the outer wrapper for the nested table content.
before
after
before
after
QA
EuiBasicTableand verify it looks and works as expectedEuiTableRowCellstory and confirm controls are working as expectedGeneral checklist
Added documentation@defaultif default values are missing) and playground togglesChecked Code Sandbox works for any docs examplesAdded or updated jest and cypress testsIf applicable, added the breaking change issue label (and filled out the breaking change checklist)If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)