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

Metric text bounces when container size changed #2238

Closed
3 tasks done
drewdaemon opened this issue Nov 9, 2023 · 3 comments · Fixed by #2270
Closed
3 tasks done

Metric text bounces when container size changed #2238

drewdaemon opened this issue Nov 9, 2023 · 3 comments · Fixed by #2270
Assignees
Labels
bug Something isn't working :chart Chart element related issue :Lens Kibana Lens related issue :metric Related to Metric chart :rendering Rendering related issues

Comments

@drewdaemon
Copy link
Contributor

drewdaemon commented Nov 9, 2023

Describe the issue
When the container size is updated along with the layout of the tiles, the text of the metric briefly changes size before adjusting itself.

To Reproduce
Steps to reproduce the behavior:

  1. Visit the metric grid story here
  2. Change the column count

Expected behaviour
The text shouldn't resize.

Screenshots

Screen.Recording.2023-11-09.at.6.22.20.AM.mov

Kibana Cross Issues
Blocking elastic/kibana#136993

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • Every related Kibana issue is listed under Kibana Cross Issues list and the kibana cross issue tag is applied
@drewdaemon drewdaemon added bug Something isn't working :chart Chart element related issue :Lens Kibana Lens related issue :metric Related to Metric chart :rendering Rendering related issues labels Nov 9, 2023
@drewdaemon
Copy link
Contributor Author

Both the data and the container dimensions are needed to compute the correct text size.

It seems like this bug is due to the calculation being run with up-to-date data and out-of-date container dimensions.

The following console statements illustrate this.

Screenshot 2023-11-09 at 8 27 24 AM

The first two lines are logged from the first parent render. You can see that a grid of width 4 is rendered with a container width of 800.

The second two are from the second parent render. Now we have a grid of width 3 being rendered with a container width of 600.

The third pair (highlighted) are from the first chart render. Here we have a grid of width 3 (new data) being rendered with an assumed width of 800 (old width). This is where we see the incorrect text size.

Then, the fourth pair are from the second chart render. It is triggered when the container width catches up to the data and the text size is corrected.

@drewdaemon
Copy link
Contributor Author

@nickofthyme any insights into why this might be happening?

@drewdaemon
Copy link
Contributor Author

drewdaemon commented Nov 10, 2023

What is going on?

@nickofthyme and I did some investigating. Here is what is roughly happening

First React render

  1. the user updates the number of columns setting
  2. container CSS and Metric data prop are updated
  3. container is resized by the browser
  4. metric renderer receives new data
  5. text size is computed (incorrectly) using old container dimensions with new data

Second React render

  1. metric renderer receives updated container dimensions
  2. text size is computed correctly, using new container dimensions and existing data

Recap

So, the problem is essentially that new data is being received a render cycle before new container dimensions.

Computing correct text size

  1. depends on the browser updating the dimensions of the parent container
  2. is tied to the React render queue instead of the browser's layout process, causing a delay

If the new data is held back until the new container dimensions are received, the text size issue isn't present, however a distorted visualization is shown when the container dimensions have been updated, but React hasn't had a chance to render yet:

Screen.Recording.2023-11-10.at.11.04.34.AM.mp4

Possible solutions

  • The client could supply expected dimensions through the React contract that are then replaced with the real container dimensions once available. In many cases, they would match or be close, so the text wouldn't need to be updated.
  • The text could be sized using CSS container queries. This would divorce text size from the React cycle and allow it to update on container resize as part of the browser's layout sequence.

@markov00 markov00 self-assigned this Dec 4, 2023
drewdaemon added a commit to elastic/kibana that referenced this issue Jan 14, 2024
## Summary

Close #136993

All charts remain the same except the following

## Metric
Each tile gets `200px` if there are multiple and `300px` if it's a
single. The default background is EUI empty.

<img width="600" alt="Screenshot 2023-10-30 at 3 55 33 PM"
src="https://github.com/elastic/kibana/assets/315764/74d7e6dc-c90a-4f60-bf56-94ab1556ad42">

<img width="600" alt="Screenshot 2023-10-30 at 3 56 36 PM"
src="https://github.com/elastic/kibana/assets/315764/3254160a-b18a-4c04-b059-f20b8f1f246a">

## XY
Vertical time-series charts have `16:9` ratio but will stretch to any
available width and won't shrink below 300px height.


https://github.com/elastic/kibana/assets/315764/3e056ae8-bc65-4851-9ad9-6c8a81bdf58a




## Gauge

Gauge gets `600px` for the long side, `300px` for the short side.

<img width="600" alt="Screenshot 2023-11-28 at 11 22 20 AM"
src="https://github.com/elastic/kibana/assets/315764/2b774515-f060-4c26-a0aa-efdeebfff0e5">

<img width="600" alt="Screenshot 2023-11-28 at 11 22 33 AM"
src="https://github.com/elastic/kibana/assets/315764/62181021-d88a-4cb6-862b-42768a2df13e">



## Known issues
- [ ] text resizing on the metric
elastic/elastic-charts#2238


https://github.com/elastic/kibana/assets/315764/0162f461-b544-44a9-971c-b2a0265d7d3a

- [x] gauge isn't showing veil on willRender

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

Flaky test runner:
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4755

---------

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: nickofthyme <[email protected]>
Co-authored-by: Stratoula Kalafateli <[email protected]>
Co-authored-by: Marta Bondyra <[email protected]>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this issue Feb 15, 2024
## Summary

Close elastic#136993

All charts remain the same except the following

## Metric
Each tile gets `200px` if there are multiple and `300px` if it's a
single. The default background is EUI empty.

<img width="600" alt="Screenshot 2023-10-30 at 3 55 33 PM"
src="https://github.com/elastic/kibana/assets/315764/74d7e6dc-c90a-4f60-bf56-94ab1556ad42">

<img width="600" alt="Screenshot 2023-10-30 at 3 56 36 PM"
src="https://github.com/elastic/kibana/assets/315764/3254160a-b18a-4c04-b059-f20b8f1f246a">

## XY
Vertical time-series charts have `16:9` ratio but will stretch to any
available width and won't shrink below 300px height.


https://github.com/elastic/kibana/assets/315764/3e056ae8-bc65-4851-9ad9-6c8a81bdf58a




## Gauge

Gauge gets `600px` for the long side, `300px` for the short side.

<img width="600" alt="Screenshot 2023-11-28 at 11 22 20 AM"
src="https://github.com/elastic/kibana/assets/315764/2b774515-f060-4c26-a0aa-efdeebfff0e5">

<img width="600" alt="Screenshot 2023-11-28 at 11 22 33 AM"
src="https://github.com/elastic/kibana/assets/315764/62181021-d88a-4cb6-862b-42768a2df13e">



## Known issues
- [ ] text resizing on the metric
elastic/elastic-charts#2238


https://github.com/elastic/kibana/assets/315764/0162f461-b544-44a9-971c-b2a0265d7d3a

- [x] gauge isn't showing veil on willRender

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

Flaky test runner:
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4755

---------

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: nickofthyme <[email protected]>
Co-authored-by: Stratoula Kalafateli <[email protected]>
Co-authored-by: Marta Bondyra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working :chart Chart element related issue :Lens Kibana Lens related issue :metric Related to Metric chart :rendering Rendering related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants