[RAC] fix the alerts table hidden rows due to miscalculated height

[ersin-erdal]

Fixes: #119310

Due to a bug in DataGrid, timeline's t_grid passes the calculated table height as prop.
It was too complex and calculating the height wrong.
Added missing pagination section height in calculation and simplified the hook.

There can be 3 conditions for the height calculation:

1. number of the alerts (rowCount) less than pageSize
2. number of the alerts (rowCount) equal to pageSize
3. number of the alerts (rowCount) greater than pageSize (does not make sense but happens during the pageSize change)

if (rowCount < pageSize){...} else {...} could cover all the cases above.
And since useLayoutEffect is a synchronised hook, setTimeout shouldn't be needed either.

But, i may miss some edge-cases, please test this and let me know if there is a missing case.

[ersin-erdal]

This is added even if there is no pagination, but since it occupies the whole page, having a gap under the table is ok.

[claudiopro]

Looks legit code wise. Can you please add a screenshot of the grid before/after the fix with a full and a half-empty page? 🙏

[janmonschke]

Hey @ersin-erdal 👋 I also created a PR for this issue. In that fix, I am simply adding an extra row height to the overall height to compensate for the miscalculation. I refrained from changing the logic too much because there seemed to be quite some edge-cases :D
Do you think the issue described in my PR is solved by your solution as well? In that case I will close my PR in favor of yours. Since the bug was also reproduced in 7.17, could you also add the v7.17.0 label to this PR?

The wrongly-calculated height issue in DataGrid has also been fixed in a recent EUI update and has been even more imrpoved in a follow-up PR so this PR does not need to target v8.1.0. In fact, after #122386 (comment) gets merged, we can get rid of the height hack once and for all which is on my todo list :)

[ersin-erdal]

Hi @janmonschke, we've been also waiting for the new EUI version.
Today i tested it with the EUI version 43.1 and since it didn't fix the bug i updated the PR (a test has been failed though!).

Unfortunately your solution wont fix the bug... it occurs when there are 2 or 3 alerts as well.
I can add v7.17.0 too, but AFAIK as we merge this PR to main, it necessarily goes to 8.1. Maybe we can remove the hack with a different PR after we merge this one.

edited: i can't add v7.17.0... it uses a different EUI version which uses a different table-row height...

[ersin-erdal]

@elasticmachine merge upstream

[kibana-ci]

💔 Build Failed

• Buildkite Build
• Commit: af0a3d7
• Interpreting CI Failures
• Storybooks Preview

Failed CI Steps

• Default CI Group #6
• Default CI Group #6

Test Failures

• [job] [logs] Default CI Group #6 / ObservabilityApp alert status filter can be filtered to only show "recovered" alerts using the filter button
• [job] [logs] Default CI Group #6 / ObservabilityApp alert status filter can be filtered to only show "recovered" alerts using the filter button

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
timelines	225.2KB	225.0KB	-210.0B

History

• 💔 Build #16540 failed d015c4c
• 💚 Build #13643 succeeded 4615fec

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

[janmonschke]

@ersin-erdal thanks for the clarification. I have since checked my solution in 8.0 against a scenario with 3 alerts on the last page and it fixes that case as well. It adds an additional row's height to make up for the miscalculation which is only wrong by about a row's height:

Screen.Recording.2022-01-11.at.11.02.12.mov

I understand that your PR also updates the height constants which is why it makes sense to not target 7.17. Do you know when the new row heights were introduced in EUI? I am struggling to find out which release it actually was. I am asking because maybe the new row height only changed in main but not in 8.0.

When I check out 8.0 locally, the row height is still 34px:

Here's the list of the different EUI versions by branches

7.17 uses EUI 39.1.3
8.0 uses EUI 40.1.1
main uses EUI 43.1.1

[janmonschke]

I have create a new PR that removes the height hack now that EUI 44.0.0 was merged into main. (#122755)
In that PR I am demonstrating that the height is rendering correctly for multiple scenarios.

Going forward I think it might make sense to close this PR then and to backport my other PR (#122542) to 8.0 and 7.17 to fix the rendering issue there.

Let me know what you think about this approach :)

[ersin-erdal]

Hi @janmonschke that totally make sense to me.
I am closing this PR in favor of yours.
Thank you again.