[Security][RAM] Fix Cases > Alerts data grid height rendering in as 0 by cee-chen · Pull Request #134268 · elastic/kibana

cee-chen · 2022-06-13T19:33:36Z

Summary

This PR should address #132901 - I don't have a Windows machine or Browserstack account to test, but I'm reasonably confident.

This PR also resolves #133691 (comment) where @XavierM was unable to add fontSize: 's' to the datagrid's gridStyle, and removes the temporary CSS workaround that was added in the interim.

Before

After

Why was this happening?

alertsCount was coming in at -1 on initial page load which was causing all kinds of rowCount / height calculation shenanigans in the data grid. Adding a conditional check to only initialize/render the data grid when alertsCount has actual data / is above -1 solves the problem.

Why is there still a slight height gap at the bottom of the datagrid in the 'after' screenshot?

I believe this is coming from two points of unnecessary display: flex usage in Security's DOM markup:

The page Wrapper component:

An EuiFlexGroup around the tabbed content:

Neither of these flex wrappers are necessary and the column flex wrapper in particular creates height expectations on the page that affect the wrapped EuiDataGrid.

As this is not an issue with EuiDataGrid itself but rather an issue with Security's markup, I have not addressed that in this PR.

Checklist

This was checked for cross-browser compatibility

`alertsCount` is -1 on init which leads to buggy data grid height calculations. Waiting for `alertsCount` to be an actual value before instantiating the grid solves the issue

…#133691 (comment)

cnasikas

Thank you for the fix!

cee-chen · 2022-06-14T15:54:40Z

Thanks a million @cnasikas! @mdefazio, do you mind approving for the .scss change whenever you have a chance? 🙏

peteharverson

Confirmed this looks good on Firefox on Windows. The table is now displayed correctly, and there is no gap below the data grid in the table in Security:

Firefox (Windows):

Is there any chance that the issue with the gap below the table in Security, on Chrome could be addressed in this PR. With these changes - Chrome (Windows):

Compared to before (Chrome, Windows):

The row height looks better with these changes, but it would be good to fix the extra padding below the table which is introduced now.

cee-chen · 2022-06-15T15:31:28Z

Thanks very much for testing on FF on Windows @petehaverson!

Is there any chance that the issue with the gap below the table in Security, on Chrome could be addressed in this PR

It looks like this is a separate issue already occurring with other Security data grids: #129057

Per my comments in the collapsed section of this PR, I strongly suspect your issues with data grid height in general across Security are due to your main page Wrapper component which sets display: flex; flex-direction: column. That flex column CSS is potentially forcing the wrapper around your datagrid(s) to be a height 0 at first init (causing miscalculations later). It's additionally not super clear to me why that flex CSS needs to be there if all you want is just a basic vertical cascade which is already the default flow for non-flex CSS.

Changing that would affect a lot more than just this data grid and I would consider that strongly out of scope for this PR.

mdefazio

LGTM!

cee-chen · 2022-06-15T15:43:25Z

Thanks all! Pulling latest for safety and then merging.

cee-chen · 2022-06-15T15:44:58Z

@elasticmachine merge upstream

peteharverson

Thanks for the extra context around the data grids in Security @constancecchen. Makes sense to address #129057 separately. LGTM

kibana-ci · 2022-06-15T16:51:08Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`triggersActionsUi`	831.9KB	831.8KB	-134.0B

History

💚 Build #50606 succeeded 640df66

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…#134268) * Fix negative rowCount causing incorrect EuiDataGrid height calculations `alertsCount` is -1 on init which leads to buggy data grid height calculations. Waiting for `alertsCount` to be an actual value before instantiating the grid solves the issue * Re-add fontSize styling which was causing grid height bugs in #133691 (comment) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit a943b81)

kibanamachine · 2022-06-15T16:57:05Z

💚 All backports created successfully

Status	Branch	Result
✅	8.3

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…#134268) (#134498) * Fix negative rowCount causing incorrect EuiDataGrid height calculations `alertsCount` is -1 on init which leads to buggy data grid height calculations. Waiting for `alertsCount` to be an actual value before instantiating the grid solves the issue * Re-add fontSize styling which was causing grid height bugs in #133691 (comment) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit a943b81) Co-authored-by: Constance <constancecchen@users.noreply.github.com>

MakoWish · 2023-06-13T17:11:24Z

This has regressed. We upgraded to 8.8.1 yesterday and are seeing this again.

cee-chen added 2 commits June 13, 2022 12:06

Fix negative rowCount causing incorrect EuiDataGrid height calculations

d1c4cf0

`alertsCount` is -1 on init which leads to buggy data grid height calculations. Waiting for `alertsCount` to be an actual value before instantiating the grid solves the issue

Re-add fontSize styling which was causing grid height bugs in elastic…

640df66

…#133691 (comment)

cee-chen requested review from XavierM, kqualters-elastic and mdefazio June 13, 2022 19:33

cee-chen marked this pull request as ready for review June 13, 2022 20:34

cee-chen requested review from a team as code owners June 13, 2022 20:34

cee-chen added release_note:skip Skip the PR/issue when compiling release notes v8.3.0 v8.4.0 bug Fixes for quality problems that affect the customer experience auto-backport Deprecated - use backport:version if exact versions are needed labels Jun 13, 2022

cnasikas approved these changes Jun 14, 2022

View reviewed changes

cnasikas mentioned this pull request Jun 14, 2022

[Security Solution][Windows: Firefox]List not showing until we go under full screen mode #132901

Closed

peteharverson reviewed Jun 15, 2022

View reviewed changes

mdefazio approved these changes Jun 15, 2022

View reviewed changes

Merge branch 'main' into security-alerts-datagrid

ed2faea

cee-chen enabled auto-merge (squash) June 15, 2022 15:45

peteharverson approved these changes Jun 15, 2022

View reviewed changes

cee-chen merged commit a943b81 into elastic:main Jun 15, 2022

cee-chen deleted the security-alerts-datagrid branch June 15, 2022 16:51

kibanamachine mentioned this pull request Jun 15, 2022

[8.3] [Security][RAM] Fix Cases > Alerts data grid height rendering in as 0 (#134268) #134498

Merged

This was referenced Jun 13, 2023

[Security Solution] Single alert may be hidden due to table height #159143

Closed

[RAM] Integration of EUI Data Grid's Custom Body Renderer for Alerts Table #159003

Closed

kqualters-elastic mentioned this pull request Jun 29, 2023

[EuiDataGrid] Use integer values for dimensions derived from integers elastic/eui#6895

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security][RAM] Fix Cases > Alerts data grid height rendering in as 0#134268

[Security][RAM] Fix Cases > Alerts data grid height rendering in as 0#134268
cee-chen merged 3 commits intoelastic:mainfrom
cee-chen:security-alerts-datagrid

cee-chen commented Jun 13, 2022 •

edited

Loading

Uh oh!

cnasikas left a comment

Uh oh!

cee-chen commented Jun 14, 2022

Uh oh!

peteharverson left a comment

Uh oh!

cee-chen commented Jun 15, 2022

Uh oh!

mdefazio left a comment

Uh oh!

cee-chen commented Jun 15, 2022 •

edited

Loading

Uh oh!

cee-chen commented Jun 15, 2022

Uh oh!

peteharverson left a comment

Uh oh!

kibana-ci commented Jun 15, 2022

Uh oh!

kibanamachine commented Jun 15, 2022

Uh oh!

MakoWish commented Jun 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

cee-chen commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Before

After

Why was this happening?

Checklist

Uh oh!

cnasikas left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Jun 14, 2022

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Jun 15, 2022

Uh oh!

mdefazio left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cee-chen commented Jun 15, 2022

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Jun 15, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

History

Uh oh!

kibanamachine commented Jun 15, 2022

💚 All backports created successfully

Questions ?

Uh oh!

MakoWish commented Jun 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cee-chen commented Jun 13, 2022 •

edited

Loading

cee-chen commented Jun 15, 2022 •

edited

Loading