bito-code-review[bot] commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4165187742
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts:
##########
@@ -74,3 +76,124 @@ export const getHeaderFontSize = (proportionValue: number)
=>
export const getComparisonFontSize = (proportionValue: number) =>
comparisonFontSizesMapping[proportionValue] ??
sharedFontSizes[sharedFontSizes.length - 1];
+
+export interface ComparisonColorTokens {
+ /** Color for the arrow indicator and (when the symbol is index 0) text. */
+ text: string;
+ /** Background color for the increase/decrease pill. */
+ background: string;
+ /** Foreground color for the increase/decrease pill's text. */
+ strongText: string;
+}
+
+/**
+ * Resolves the increase/decrease colors to use for rendering, given the
+ * chart's current `increaseColor` / `decreaseColor` (from the
+ * `ColorPickerControl`s added after this became customizable) and the
+ * legacy `comparisonColorScheme` field.
+ *
+ * Charts saved before `increaseColor` / `decreaseColor` existed only have
+ * `comparisonColorScheme`, a 2-choice select ('Green' | 'Red') where 'Green'
+ * meant "green for increase, red for decrease" and 'Red' meant the reverse.
+ * Both legacy choices map onto the same 'Green' | 'Red' semantic token names
+ * used by the new controls' presets, so resolving through it here
+ * reproduces the exact old behavior (including the reversed case) without a
+ * data migration.
+ */
+export const resolveComparisonColorKeys = (
+ comparisonColorScheme: string | undefined,
+ increaseColor: string | undefined,
+ decreaseColor: string | undefined,
+): { increaseColor: string; decreaseColor: string } => {
+ const legacyReversed = comparisonColorScheme === ColorSchemeEnum.Red;
+ return {
+ increaseColor:
+ increaseColor ??
+ (legacyReversed ? ColorSchemeEnum.Red : ColorSchemeEnum.Green),
+ decreaseColor:
+ decreaseColor ??
+ (legacyReversed ? ColorSchemeEnum.Green : ColorSchemeEnum.Red),
+ };
+};
+
+/**
+ * Hex alpha suffix appended to a custom comparison color to build the pill
+ * background tint: 0x1A / 0xFF is roughly 10% opacity.
+ */
+export const COMPARISON_TINT_ALPHA_HEX = '1A';
+
+/**
+ * Resolves a single color value (semantic token name or literal hex from
+ * the color picker) to the (arrow/text, background, strong-text) triad used
+ * across the comparison pills. 'Green' / 'Red' keep using the paired
+ * success/error theme tokens exactly as before these colors were
+ * customizable; any other value is either a theme token name (e.g.
+ * 'colorPrimary', emitted by the picker's `resolveThemeTokens` option) or a
+ * literal hex -- 6-digit, or 8-digit when the alpha-enabled picker is used
+ * -- in which case the background is a light (~10% opacity) tint of that
+ * same color.
+ */
+export const getComparisonColorTokens = (
+ colorValue: string,
+ theme: SupersetTheme,
+): ComparisonColorTokens => {
+ if (colorValue === ColorSchemeEnum.Green) {
+ return {
+ text: theme.colorSuccess,
+ background: theme.colorSuccessBg,
+ strongText: theme.colorSuccessText,
+ };
+ }
+ if (colorValue === ColorSchemeEnum.Red) {
+ return {
+ text: theme.colorError,
+ background: theme.colorErrorBg,
+ strongText: theme.colorErrorText,
+ };
+ }
+ const themeColors = theme as unknown as Record<string, string>;
Review Comment:
<!-- Bito Reply -->
The suggestion to use `Record<string, unknown>` and a `typeof === 'string'`
guard is correct and improves type safety. It avoids the unsoundness of the
double assertion `theme as unknown as Record<string, string>` by ensuring that
non-string fields in `SupersetTheme` are not incorrectly treated as strings,
which aligns with the project's type-safety standards.
**superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts**
```
const themeColors = theme as unknown as Record<string, unknown>;
if (typeof themeColors[colorValue] === 'string') {
return {
text: themeColors[colorValue] as string,
background: `${themeColors[colorValue]}${COMPARISON_TINT_ALPHA_HEX}`,
strongText: themeColors[colorValue] as string,
};
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
