-
Notifications
You must be signed in to change notification settings - Fork 805
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
[VisBuilder][BUG] Flat render structure in Metric and Table Vis #6674
base: main
Are you sure you want to change the base?
Conversation
This issue is caused by the callback behavior in ReactExpressionRenderer. The callback to ReactExpressionRenderer to update isloading state is lost if we wrap the render vis with VisualizationContainer. This PR moved VisualizationContainer directly in MetricVisComponent. When put VisualizationContainer directly in the MetricVisComponent, all the lifecycle methods and hooks within MetricVisComponent directly influence the rendering of VisualizationContainer. This means that calls to renderComplete and other lifecycle integrations are more directly managed. Issue Resolved: opensearch-project#6671 Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6674 +/- ##
==========================================
+ Coverage 67.79% 68.18% +0.39%
==========================================
Files 3413 2679 -734
Lines 66755 50961 -15794
Branches 10861 9171 -1690
==========================================
- Hits 45254 34749 -10505
+ Misses 18857 14013 -4844
+ Partials 2644 2199 -445
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we write a simple unit test for this?
Table is good but metrics has some embeddable issue When load the first metric: metrics-1.movCheck metrics vis and see it actually has no problem metrics-2.movThen if load another metric vis, no issue at all |
@ananzh is this PR targeting 2.15, could you check the failed workflow and see if related to your change? |
Description
This issue is caused by the callback behavior in ReactExpressionRenderer. The callback to ReactExpressionRenderer to update
isLoading
state is lost if we wrap the render vis with VisualizationContainer.This PR moved VisualizationContainer directly in MetricVisComponent. When put VisualizationContainer directly in the MetricVisComponent, all the lifecycle methods and hooks within MetricVisComponent directly influence the rendering of VisualizationContainer. This means that calls to renderComplete and other lifecycle integrations are more directly managed.
Issues Resolved
#6671
Screenshot
Changelog
Check List
yarn test:jest
yarn test:jest_integration