bito-code-review[bot] commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4140298863
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts:
##########
@@ -74,3 +76,97 @@ 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>;
+ const resolvedColor = Object.prototype.hasOwnProperty.call(
+ themeColors,
+ colorValue,
+ )
+ ? themeColors[colorValue]
+ : colorValue;
+ // An 8-digit hex (alpha-enabled picker) already carries its own alpha
+ // channel; strip it before appending the tint suffix below so the
+ // background stays a valid 8-digit hex instead of stacking a second one.
+ const opaqueColor = /^#[0-9a-f]{8}$/i.test(resolvedColor)
+ ? resolvedColor.slice(0, 7)
+ : resolvedColor;
+ return {
+ text: resolvedColor,
+ background: `${opaqueColor}${COMPARISON_TINT_ALPHA_HEX}`,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Invalid CSS tint on rgba tokens</b></div>
<div id="fix">
Theme tokens resolved via `resolveThemeTokens` can hold `rgba()` strings
(e.g. `colorText` is `rgba(255,255,255,0.85)` in the dark theme), and
`getComparisonColorTokens` unconditionally appends `COMPARISON_TINT_ALPHA_HEX`
to any non-8-digit-hex value, producing invalid CSS like `rgba(...)1A` for the
pill background. Restrict the tint suffix to 6-digit hex values and pass other
formats through unchanged.
</div>
</div>
<small><i>Code Review Run #e33986</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/controls/BulletRangeColorsControl/index.tsx:
##########
@@ -0,0 +1,107 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { t } from '@apache-superset/core/translation';
+import { styled } from '@apache-superset/core/theme';
+import { Button } from '@superset-ui/core/components';
+import { tokenizeToNumericArray } from '@superset-ui/plugin-chart-echarts';
+import ControlHeader from '../../ControlHeader';
+import ColorPickerControl from '../ColorPickerControl';
+import type { ColorPickerValue } from '../ColorPickerControl';
+import { BulletRangeColorsControlProps } from './types';
+
+const RangeRow = styled.div`
+ display: flex;
+ align-items: center;
+ gap: ${({ theme }) => theme.sizeUnit * 2}px;
+ margin-bottom: ${({ theme }) => theme.sizeUnit}px;
+`;
+
+const RangeLabel = styled.span`
+ min-width: 90px;
+ color: ${({ theme }) => theme.colorTextSecondary};
+ font-size: ${({ theme }) => theme.fontSizeSM}px;
+`;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated sibling control logic</b></div>
<div id="fix">
This new control duplicates the sibling `IntervalColorsControl` almost
verbatim: identical `RangeRow`/`RangeLabel` styled CSS, the same
`handleColorChange` (map over parsed values, swap one index), and the same
`ColorPickerControl outputFormat="hex"` row rendering. A bug fix or behavior
change in one will silently diverge from the other. Consider extracting the
shared row/color-picker logic.
</div>
</div>
<small><i>Code Review Run #e33986</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.test.tsx:
##########
@@ -0,0 +1,107 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { render, screen } from 'spec/helpers/testing-library';
+import PopKPI from './PopKPI';
+import { PopKPIProps } from './types';
+
+const baseProps: PopKPIProps = {
+ height: 200,
+ width: 400,
+ data: [],
+ metrics: [],
+ metricName: 'Sales',
+ showMetricName: true,
+ headerText: '',
+ boldText: true,
+ bigNumber: '120',
+ prevNumber: '100',
+ valueDifference: '20',
+ percentDifferenceFormattedString: '20%',
+ compType: 'r',
+ percentDifferenceNumber: 0.2,
+ subtitleFontSize: 0.15,
+ shift: '',
+ headerFontSize: 'l',
+ subheaderFontSize: 'm',
+ comparisonColorEnabled: true,
+};
+
+test('renders the up arrow and metric values when the metric increased', () =>
{
+ render(<PopKPI {...baseProps} />);
+
+ expect(screen.getByText('↑')).toBeInTheDocument();
+ expect(screen.getByText('120')).toBeInTheDocument();
+ expect(screen.getByText('Sales')).toBeInTheDocument();
+});
+
+test('renders the down arrow when the metric decreased', () => {
+ render(
+ <PopKPI
+ {...baseProps}
+ percentDifferenceNumber={-0.2}
+ valueDifference="-20"
+ />,
+ );
+
+ expect(screen.getByText('↓')).toBeInTheDocument();
+ expect(screen.getByText('-20')).toBeInTheDocument();
+});
+
+// The precise color resolution (legacy `comparisonColorScheme` fallback,
+// custom hex precedence, the neutral disabled state) is covered by pure
+// unit tests against `resolveComparisonColorKeys` / `getComparisonColorTokens`
+// in utils.test.ts, since emotion's `css` prop styles aren't reliably
+// observable through jsdom's computed styles. These tests instead confirm
+// PopKPI renders without crashing across the same prop combinations.
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inaccurate jsdom styles comment</b></div>
<div id="fix">
The justification that emotion's `css` prop styles 'aren't reliably
observable through jsdom's computed styles' is inaccurate:
`@emotion/jest@^11.14.2` is a devDependency in `superset-frontend/package.json`
and provides `toHaveStyleRule` for exactly this. The comment misleads
maintainers into accepting the mount-only sibling tests as the best possible
coverage.
</div>
</div>
<small><i>Code Review Run #e33986</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
