[代理镜像] feat(plugins): Adding colors to BigNumber with Time Comparison chart by Antonio-RiveroMartnez · Pull Request #27052 · apache/superset

Antonio-RiveroMartnez · 2024-02-08T14:22:10Z

SUMMARY

Currently our BigNumber with Time Comparison chart has no visual indicators to help user easily tell when the differences are positive or negatives. This PR adds a customize control to enable colors in the chart.

Also this PR is updating the thumbnail to reflect an up to date version of the chart with these colors.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

- Add new checbox control so users can toggle colors in the chart - Make the component capable of using the right colors based on the data - Fix types in props ofthe component - Update the thumbnail of the chart

codecov · 2024-02-08T14:48:58Z

Codecov Report

Attention: 124 lines in your changes are missing coverage. Please review.

Comparison is base (daaf657) 67.19% compared to head (6afe5a6) 67.16%.
Report is 21 commits behind head on master.

Files	Patch %	Lines
...s/plugin-chart-period-over-period-kpi/src/utils.ts	3.09%	94 Missing ⚠️
...plugin-chart-period-over-period-kpi/src/PopKPI.tsx	0.00%	27 Missing ⚠️
...rt-period-over-period-kpi/src/plugin/buildQuery.ts	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #27052      +/-   ##
==========================================
- Coverage   67.19%   67.16%   -0.03%     
==========================================
  Files        1899     1900       +1     
  Lines       74369    74423      +54     
  Branches     8274     8293      +19     
==========================================
+ Hits        49971    49988      +17     
- Misses      22343    22380      +37     
  Partials     2055     2055

Flag	Coverage Δ
javascript	`56.87% <2.36%> (-0.04%)`	⬇️

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.

kgabryje · 2024-02-08T15:23:24Z

+        comparisonColorEnabled ? backgroundColor : defaultBackgroundColor
+      };
+      color: ${comparisonColorEnabled ? textColor : defaultTextColor};
+      padding: 2px 5px;


could we use full grid units? Or does it look weird?

kgabryje · 2024-02-08T15:28:16Z


+const SymbolWrapper = styled.div<PopKPIComparisonSymbolStyleProps>`
+  ${({ theme, percentDifferenceNumber, comparisonColorEnabled }) => {
+    const defaultBackgroundColor = theme.colors.grayscale.light4;


Interesting approach with putting js inside of styled component like that! But I wonder if deducing colours from more "high-level" data belongs to a styled component. Maybe we should move that logic to PopKPI and just pass backgroundColor and textColor to SymbolWrapper?
But if you think this solution is better then I won't argue, I don't have a strong opinion here

hehe yeah, I had it like that before and moved to this, it doesn't feel quite "well" but now that you noticed too, let's move it there instead. Thanks

rusackas · 2024-02-08T15:30:44Z

I'm trying to upgrade Storybook, but ironing out a few last details. When that's done, we should add this plugin to Storybook and make sure it has controls for these options

Antonio-RiveroMartnez · 2024-02-08T15:32:00Z

I'm trying to upgrade Storybook, but ironing out a few last details. When that's done, we should add this plugin to Storybook and make sure it has controls for these options

Yes! This will be included and we'll have more docs about it too. Right now it's behind Feature Flag as experimental, but an update is coming soon on that regard. Thanks

kgabryje · 2024-02-08T15:33:48Z

+                ? prevNumber
+                : index === 1
+                ? valueDifference
+                : percentDifferenceFormattedString}


Hmm I'm not sure about coupling the values with indexes like this. I think it would be cleaner if we had some key-value mapping between symbols and values, or an array like [['#', prevNumber], ['△', valueDifference], ['%', percentDifferenceFormattedString]]

kgabryje · 2024-02-08T15:36:10Z

+    prevNumber: string;
+    valueDifference: string;
+    percentDifferenceFormattedString: string;
    compType: String;


can we also fix those capital S strings? 😛

- Move background and textcolor computation inside the main component so the Styled doesnt have to deal with logic - Stop accessing values with indexes and have a single array that collects both the symbol and the value - fix more typing

Antonio-RiveroMartnez · 2024-02-08T16:52:42Z