[EuiDataGrid] Various row height fixes #8025
Conversation
…duction usages - including complex children such as checkboxes & badges
- content is no longer inside a button, so text align is no longer needed and makes CSS overrides a little easier
… single to multi row lines
- we're only rendering 5 rows so index 5 wasn't doing anything / applying to any row. this also clearly reveals a bug with lineCount: 2 - `onChange` needs to be reset to undefined because of `enableFunctionToggleControls`+`controls.include`
😭 not sure what past-me 3 years ago was smoking, but the `excludePadding` prop is no longer needed
cee-chen
force-pushed
the
datagrid/linecount-1
branch
from
52bb4d5
to
e1a225e
Compare
…ons.lineHeight` change - requires waterfalling `gridStyles` down to EuiDataGridCell
- remove the cached styles and just use `window.getComputedStyles` - which causes a reflow, but since we need it for lineHeight in any case we might as well keep using it 🤷
cee-chen
force-pushed
the
datagrid/linecount-1
branch
from
b8d5c6a
to
4ccf3a1
Compare
packages/eui/src/components/datagrid/data_grid.stories.utils.tsx
Outdated
Show resolved
Hide resolved
| autoHeight: css` | ||
| ${logicalCSS('height', 'auto')} | ||
| `, | ||
| defaultHeight: css` | ||
| ${logicalCSS('height', '100%')} | ||
| `, | ||
| // Control columns should be vertically centered with the *first line* of text | ||
| // for both single and multi-line heights (see https://github.com/elastic/eui/issues/7897) | ||
| controlColumn: css` |
There was a problem hiding this comment.
Should we align the content horizontally as well?
Currently the controls in the story examples are not centered for cellPadding="s | l"
There was a problem hiding this comment.
I noticed this too and I'm debating a separate PR for that where controlColumns.width does not include the cell padding, so that it scales with different densities and this off-centeredness doesn't occur.
Center alignment would solve the issue for the compact density, but not the expanded density, since technically the width provided is too small for both the expanded cell padding + the checkbox width.
Either way I'd like to address that separately from this PR, since that has more to do with column widths and less to do with row heights, and changing how the width API works for control columns would technically be a breaking change.
There was a problem hiding this comment.
Thanks for the context! I'm fine with addressing it in a separate PR then 👍
💚 Build Succeeded
History
|
|
Thanks a million as always for the thorough review/QA Lene! |
Summary
fontSize: "s"#7780 (comment)This PR contains several fixes as well as storybook/VRT test updates. I strongly recommend following along by commit, as I continued to update VRT screenshots as I fixed bugs, which is helpful in seeing the visual diffs.
QA
5row heighttoCustom(2 lines)SingleandCustomfontSizeandcellPaddingoption permutations and confirm that row height(s) update accordingly/as expectedCompactand the row height toCustomwith1line per rowGeneral checklist
- [ ] Checked in both light and dark modes- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)