-
Notifications
You must be signed in to change notification settings - Fork 14k
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(plugin-chart-period-over-period-kpi): Blank chart when switching from BigNumberTotal #27203
Conversation
85a0549
to
3522242
Compare
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.
❤️ thanks for making these changes, I left a couple comments but nothing blocking.
...ntend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/index.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #27203 +/- ##
==========================================
+ Coverage 69.58% 69.68% +0.10%
==========================================
Files 1905 1908 +3
Lines 74580 74517 -63
Branches 8330 8309 -21
==========================================
+ Hits 51893 51924 +31
+ Misses 20637 20540 -97
- Partials 2050 2053 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
🏷️ preset:2024.8 |
…from BigNumberTotal (apache#27203) (cherry picked from commit 5403797)
…from BigNumberTotal (apache#27203) (cherry picked from commit 5403797)
…from BigNumberTotal (apache#27203) (cherry picked from commit 5403797)
…from BigNumberTotal (apache#27203) (cherry picked from commit 5403797)
…from BigNumberTotal (apache#27203) (cherry picked from commit 5403797)
…from BigNumberTotal (apache#27203) (cherry picked from commit 5403797)
…from BigNumberTotal (apache#27203) (cherry picked from commit 5403797)
…from BigNumberTotal (apache#27203) (cherry picked from commit 5403797)
…from BigNumberTotal (apache#27203)
…from BigNumberTotal (apache#27203) (cherry picked from commit 5403797)
…from BigNumberTotal (apache#27203) (cherry picked from commit 5403797)
…from BigNumberTotal (apache#27203) (cherry picked from commit 5403797)
…from BigNumberTotal (apache#27203) (cherry picked from commit 5403797)
…from BigNumberTotal (apache#27203) (cherry picked from commit 5403797)
…from BigNumberTotal (apache#27203)
SUMMARY
When user creates and saves a Big Number chart and then switches to Big Number Period over Period, the chart appears blank. The reason for that is that Period over Period chart expects the values coming from
header_font_size
control to be pixels. However, the old Big Number chart uses proportions (0.2-0.6) in that control, so when the new chart inherits those values, the size in pixels is <1, so it seems blank.This PR implements a mapping between proportion values used in Big Number and actual pixel size values expected by Period over Period chart.
Additionally, this PR moves the KPI chart to a common directory with other BigNumber plugins so that they can share to font size controls.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Before:
Screen.Recording.2024-02-22.at.14.43.40.mov
After:
Screen.Recording.2024-02-22.at.14.42.56.mov
ADDITIONAL INFORMATION