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

fix: use passed size in pixel without waiting for resizeObserver #2270

Merged
merged 17 commits into from
Jan 31, 2024

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Dec 6, 2023

Summary

This change bypasses the need for one ResizeObserver event when passing chart size in pixel.
This avoid double re-renders with different sizes in the case of a datum + size change from the caller component.

Issues

fix #2238
fix #909

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • Unit tests have been added or updated to match the most common scenarios
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)

@markov00 markov00 added bug Something isn't working :chart Chart element related issue :all Applies to all chart types :metric Related to Metric chart labels Dec 6, 2023
@markov00
Copy link
Member Author

markov00 commented Dec 6, 2023

buildkite test this

@markov00 markov00 marked this pull request as ready for review December 7, 2023 08:42
Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

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

Hmm, I still see the text bounce... did I miss something?

Screen.Recording.2023-12-07.at.8.29.31.AM.mov

@markov00
Copy link
Member Author

markov00 commented Dec 7, 2023

Hmm, I still see the text bounce... did I miss something?

Correct, I may have messed up with something here, let me check again thanks for the heads up

@markov00
Copy link
Member Author

markov00 commented Dec 7, 2023

buildkite update screenshots

@markov00
Copy link
Member Author

markov00 commented Dec 7, 2023

@drewdaemon ok so I've adjusted the example in Storybook, it works fine now:

  • if the chart is resizable (like a panel in a dashboard) external container and the Chart size should be configured at 100% width/height
  • if the sizing is constrained then both the container and the Chart size need to be configured in pixels

@drewdaemon
Copy link
Contributor

The changes to the storybook may mark a departure from how this will be used in Kibana.

The way we have things set up creates a delay between when we apply the new CSS to the parent container and when we find out the true container dimensions the browser assigned.

For example, I could set the CSS on the parent container to the following

    height: 100%;
    width: 100%;
    max-width: 200px;
    max-height: 1200px 

Here I'm requesting a width of 200px and a height of 1200px. But, the parent of the parent container may only be 600px tall.

So, the application code would be telling Elastic charts that the text size should be based on a 200x1200 container when it's really in a 200x600 container.

If the client detects that the true size is 200x600 and tells Elastic charts, the text size will be corrected. But this just leads to the same bouncing behavior as we see today.

This is the reason I wondered about using CSS container queries to decouple the text size from the React render cycle.

As far as I can see, forcing the client to choose between "resizable" mode and "static" mode doesn't fit the experience we're trying to deliver.

Might be easiest to sync on this problem. Maybe there's some change I can make on my end as well.

@markov00
Copy link
Member Author

The changes to the storybook may mark a departure from how this will be used in Kibana.

The way we have things set up creates a delay between when we apply the new CSS to the parent container and when we find out the true container dimensions the browser assigned.

For example, I could set the CSS on the parent container to the following

    height: 100%;
    width: 100%;
    max-width: 200px;
    max-height: 1200px 

Here I'm requesting a width of 200px and a height of 1200px. But, the parent of the parent container may only be 600px tall.

So, the application code would be telling Elastic charts that the text size should be based on a 200x1200 container when it's really in a 200x600 container.

If the client detects that the true size is 200x600 and tells Elastic charts, the text size will be corrected. But this just leads to the same bouncing behavior as we see today.

This is the reason I wondered about using CSS container queries to decouple the text size from the React render cycle.

As far as I can see, forcing the client to choose between "resizable" mode and "static" mode doesn't fit the experience we're trying to deliver.

Might be easiest to sync on this problem. Maybe there's some change I can make on my end as well.

@drewdaemon let's sync on this together, I'm not fully sure I'm aligned with how you are currently handing this in Kibana

@markov00 markov00 force-pushed the 2023_12_06-fix_metric_double_rendering branch 2 times, most recently from 4163845 to cf41fa0 Compare December 19, 2023 17:03
@markov00 markov00 force-pushed the 2023_12_06-fix_metric_double_rendering branch from cf41fa0 to 19f7f4c Compare December 19, 2023 18:18
@markov00
Copy link
Member Author

markov00 commented Jan 9, 2024

just waiting on elastic/kibana#173772 to be merged

@markov00
Copy link
Member Author

@nickofthyme can you review this please?

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Looks good to me, I don't see any issues with these change in code or in storybook.

packages/charts/src/components/chart.tsx Outdated Show resolved Hide resolved
packages/charts/api/charts.api.md Outdated Show resolved Hide resolved
packages/charts/api/charts.api.md Outdated Show resolved Hide resolved
@markov00 markov00 merged commit f9c11fc into elastic:main Jan 31, 2024
13 checks passed
@markov00 markov00 deleted the 2023_12_06-fix_metric_double_rendering branch January 31, 2024 09:11
nickofthyme pushed a commit that referenced this pull request Feb 13, 2024
# [64.0.0](v63.1.0...v64.0.0) (2024-02-13)

### Bug Fixes

* **deps:** update dependency @elastic/eui to ^93.1.0 ([#2332](#2332)) ([855e357](855e357))
* **deps:** update dependency @elastic/eui to v93 ([#2324](#2324)) ([ce19379](ce19379))
* **deps:** update dependency @playwright/test to ^1.41.2 ([#2330](#2330)) ([1f8c638](1f8c638))
* **metric:** move `MetricSpec.body` to `MetricBase` ([#2336](#2336)) ([3390e25](3390e25))
* use passed size in pixel without waiting for resizeObserver ([#2270](#2270)) ([f9c11fc](f9c11fc))

### chore

* **bullet:** bullet improvements, bug fixes and renaming ([#2319](#2319)) ([34fd38b](34fd38b))

### BREAKING CHANGES

* **metric:** Moves `MetricSpec.body` to `MetricDatum.body`/`MetricBase.body`
* **bullet:** Rename `BulletGraph` to `Bullet` and `ColorBandSimpleConfig.classes` to `steps`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:all Applies to all chart types bug Something isn't working :chart Chart element related issue :metric Related to Metric chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metric text bounces when container size changed Improve popper.js performance with removing debounce.
3 participants