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 visualization chart not expanded after legend toggle #9170

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

wanglam
Copy link
Contributor

@wanglam wanglam commented Jan 10, 2025

Description

This PR fixes an issue where the chart was not expanding after hiding the legend. The root cause was that the VisLegend component was using vis.getUiState() as the uiState in the vis_controller, which was different from the uiState in the VisualizationChart component. According to the code in visualization_chart, it was using this.props.uiState. Therefore, changes to vis.legendOpen were not triggering a re-render in VisualizationChart.

To resolve this issue, I added a uiState synchronization logic in src/plugins/visualizations/public/expressions/visualization_renderer.tsx. This logic compares the visState in vis and calls setState to ensure that the two uiState instances are the same. As a result, the chart will be resized correctly.

Issues Resolved

#9143

Testing the changes

  1. Import "Sample web logs" sample data
  2. Create a new dashboard with "(Line) Avg bytes over time"
  3. Update a close time range like "15 seconds ago", the chart will become no results like screenshot below
    image
  4. Update time range to very early like "15 years ago"
  5. Click "Toggle legend" on the bottom left, the chart should expanded after legend hide
    image

Changelog

  • fix: Visualization chart not expanded after legend toggle

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Signed-off-by: Lin Wang <[email protected]>
Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.05%. Comparing base (e1b12ab) to head (337ef1c).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9170      +/-   ##
==========================================
+ Coverage   61.01%   61.05%   +0.03%     
==========================================
  Files        3813     3813              
  Lines       91401    91403       +2     
  Branches    14443    14444       +1     
==========================================
+ Hits        55772    55805      +33     
+ Misses      32067    32028      -39     
- Partials     3562     3570       +8     
Flag Coverage Δ
Linux_1 29.13% <100.00%> (+0.04%) ⬆️
Linux_2 56.45% <ø> (ø)
Linux_3 38.03% <0.00%> (-0.01%) ⬇️
Linux_4 29.03% <ø> (ø)
Windows_1 29.14% <100.00%> (+0.04%) ⬆️
Windows_2 56.40% <ø> (ø)
Windows_3 38.03% <0.00%> (-0.01%) ⬇️
Windows_4 29.03% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Nice! Thanks :)

@@ -57,6 +57,10 @@ export const visualization = (): ExpressionRenderDefinition<VisRenderValue> => (
unmountComponentAtNode(domNode);
});

if (vis.getUiState() !== uiState) {
vis.setUiState(uiState);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am concerned that handlers.uiState has a type of any while vis.setUiState is expecting PersistedState.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, but I think it's not a big issue. According the implementation in the Visualization component, the uiState has already been assigned to the vis object. The vis.uiState always be an empty since vis object recreated in every render. To address this any concern, how about add a type assertment in the compare condition. Refactor code like below:

if (uiState instanceof PersistedState && vis.getUiState() !== uiState) {
      vis.setUiState(uiState);
}

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Should we do a deep compare?

Copy link
Contributor Author

@wanglam wanglam Jan 15, 2025

Choose a reason for hiding this comment

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

vis.getUiState() should always be an empty object in this place. Since we construct a new ExpVis object above. The uiState of this object should be empty too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants