search

elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.71k stars 8.12k forks source link

[APM] Reset container height of trace waterfall #150542

Closed dgieselaar closed 1 week ago

dgieselaar commented 1 year ago

Currently, the container of the trace waterfall is not reset (fit to content) when navigating traces, which can result in a lot of blank space:

CleanShot 2023-02-08 at 12 20 51@2x

We have a resetting height container that we can use: https://github.com/elastic/kibana/blob/main/x-pack/plugins/apm/public/components/shared/height_retainer/resetting_height_container.tsx

elasticmachine commented 1 year ago

Pinging @elastic/apm-ui (Team:APM)

sorenlouv commented 1 year ago

The purpose of the HeightRetainer hook is to make sure the page doesn't shrink vertically causing the scroll position to change. I'm not sure what the purpose of ResettingHeightRetainer is.

I think we can improve the HeightRetainer so it only retains the height down to where the user's bottom screen is (a little hard to explain - might be easier to talk out over zoom).

dgieselaar commented 1 year ago

@sqren I think the HeightRetainer component only grows vertically. IMHO your suggested solution is a bit hackish. The ResettingHeightContainer "forgets" about a fixed height as soon as the content has settled. E.g., if you show content of 800px, and then a loading spinner, it will keep the container at 800px, and then when content comes back in at 400px, it will understand that it needs to update its minHeight to 400px. The HeightRetainer component would keep it at 800px. IMHO we should replace the HeightRetainer implementation with the one from ResettingHeightContainer.

sorenlouv commented 1 year ago

ResettingHeightContainer "forgets" about a fixed height as soon as the content has settled. E.g., if you show content of 800px, and then a loading spinner, it will keep the container at 800px, and then when content comes back in at 400px, it will understand that it needs to update its minHeight to 400px.

Ah, agree, this sounds better.

IMHO we should replace the HeightRetainer implementation with the one from ResettingHeightContainer.

Yes, I didn't understand why we have two very similar approaches. If we can replace HeightRetainer with ResettingHeightContainer I'm all for it.

elasticmachine commented 2 months ago

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

crespocarlos commented 1 week ago

I'm almost sure this has been fixed