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

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

Merged
merged 3 commits into from
Jun 15, 2022

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Jun 13, 2022

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:

  1. The page Wrapper component:
  1. 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

cee-chen added 2 commits June 13, 2022 12:06
`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
@cee-chen cee-chen marked this pull request as ready for review June 13, 2022 20:34
@cee-chen cee-chen requested review from a team as code owners June 13, 2022 20:34
@cee-chen 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
Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

@cee-chen
Copy link
Contributor Author

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

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

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):
image

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):

image

Compared to before (Chrome, Windows):
image

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
Copy link
Contributor Author

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.

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

LGTM!

@cee-chen
Copy link
Contributor Author

cee-chen commented Jun 15, 2022

Thanks all! Pulling latest for safety and then merging.

@cee-chen
Copy link
Contributor Author

@elasticmachine merge upstream

@cee-chen cee-chen enabled auto-merge (squash) June 15, 2022 15:45
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

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

@kibana-ci
Copy link
Collaborator

💚 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

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

@cee-chen cee-chen merged commit a943b81 into elastic:main Jun 15, 2022
@cee-chen cee-chen deleted the security-alerts-datagrid branch June 15, 2022 16:51
kibanamachine pushed a commit that referenced this pull request Jun 15, 2022
…#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
Copy link
Contributor

💚 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

kibanamachine added a commit that referenced this pull request Jun 15, 2022
…#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
Copy link

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

table_too_small

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes v8.3.0 v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants