Skip to content
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.

Sign up for GitHub

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

feat(plugins): Adding colors to BigNumber with Time Comparison chart #27052

Merged
merged 4 commits into from
Feb 12, 2024
Merged

feat(plugins): Adding colors to BigNumber with Time Comparison chart #27052

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4be0880
BigNumber with Time Comparison:
Antonio-RiveroMartnez Feb 8, 2024
82f0eeb
BigNumber with Time Comparison:
Antonio-RiveroMartnez Feb 8, 2024
fdb7aa7
BigNumber with Time Comparison:
Antonio-RiveroMartnez Feb 12, 2024
6afe5a6
BigNumber with Time Comparison:
Antonio-RiveroMartnez Feb 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 81 additions & 15 deletions superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@
*/
import React, { createRef } from 'react';
import { css, styled, useTheme } from '@superset-ui/core';
import { PopKPIComparisonValueStyleProps, PopKPIProps } from './types';
import {
PopKPIComparisonSymbolStyleProps,
PopKPIComparisonValueStyleProps,
PopKPIProps,
} from './types';

const ComparisonValue = styled.div<PopKPIComparisonValueStyleProps>`
${({ theme, subheaderFontSize }) => `
Expand All @@ -30,16 +34,51 @@
`}
`;

const SymbolWrapper = styled.div<PopKPIComparisonSymbolStyleProps>`

Check warning on line 37 in superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx

View check run for this annotation

Codecov / codecov/patch

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx#L37 

Added line #L37 was not covered by tests
${({ theme, percentDifferenceNumber, comparisonColorEnabled }) => {
const defaultBackgroundColor = theme.colors.grayscale.light4;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const defaultTextColor = theme.colors.grayscale.base;
const isPositiveChange = percentDifferenceNumber > 0;

Check warning on line 41 in superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx

View check run for this annotation

Codecov / codecov/patch

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx#L39-L41 

Added lines #L39 - L41 were not covered by tests
const backgroundColor =
percentDifferenceNumber === 0
? defaultBackgroundColor
: isPositiveChange
? theme.colors.success.light2
: theme.colors.error.light2;
const textColor =
percentDifferenceNumber === 0
? defaultTextColor
: isPositiveChange
? theme.colors.success.base
: theme.colors.error.base;

return `

Check warning on line 55 in superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx

View check run for this annotation

Codecov / codecov/patch

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx#L55 

Added line #L55 was not covered by tests
background-color: ${
comparisonColorEnabled ? backgroundColor : defaultBackgroundColor
};
color: ${comparisonColorEnabled ? textColor : defaultTextColor};
padding: 2px 5px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

border-radius: ${theme.gridUnit * 2}px;
display: inline-block;
margin-right: ${theme.gridUnit}px;
`;
}}
`;

const SYMBOLS = ['#', '△', '%'];

Check warning on line 68 in superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx

View check run for this annotation

Codecov / codecov/patch

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx#L68 

Added line #L68 was not covered by tests

export default function PopKPI(props: PopKPIProps) {
const {
height,
width,
bigNumber,
prevNumber,
valueDifference,
percentDifference,
percentDifferenceFormattedString,
headerFontSize,
subheaderFontSize,
comparisonColorEnabled,
percentDifferenceNumber,
} = props;

const rootElem = createRef<HTMLDivElement>();
Expand All @@ -63,9 +102,28 @@
text-align: center;
`;

const getArrowIndicatorColor = () => {

Check warning on line 105 in superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx

View check run for this annotation

Codecov / codecov/patch

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx#L105 

Added line #L105 was not covered by tests
if (!comparisonColorEnabled) return theme.colors.grayscale.base;
return percentDifferenceNumber > 0
? theme.colors.success.base
: theme.colors.error.base;
};

const arrowIndicatorStyle = css`

Check warning on line 112 in superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx

View check run for this annotation

Codecov / codecov/patch

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx#L112 

Added line #L112 was not covered by tests
color: ${getArrowIndicatorColor()};
margin-left: ${theme.gridUnit}px;
`;

return (
<div ref={rootElem} css={wrapperDivStyles}>
<div css={bigValueContainerStyles}>{bigNumber}</div>
<div css={bigValueContainerStyles}>
{bigNumber}
{percentDifferenceNumber !== 0 && (
<span css={arrowIndicatorStyle}>
{percentDifferenceNumber > 0 ? '↑' : '↓'}
</span>
)}
</div>
<div
css={css`
width: 100%;
Expand All @@ -77,18 +135,26 @@
display: table-row;
`}
>
<ComparisonValue subheaderFontSize={subheaderFontSize}>
{' '}
#: {prevNumber}
</ComparisonValue>
<ComparisonValue subheaderFontSize={subheaderFontSize}>
{' '}
Δ: {valueDifference}
</ComparisonValue>
<ComparisonValue subheaderFontSize={subheaderFontSize}>
{' '}
%: {percentDifference}
</ComparisonValue>
{SYMBOLS.map((symbol, index) => (
<ComparisonValue

Check warning on line 139 in superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx

View check run for this annotation

Codecov / codecov/patch

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx#L139 

Added line #L139 was not covered by tests
key={`comparison-symbol-${symbol}`}
subheaderFontSize={subheaderFontSize}
>
<SymbolWrapper
percentDifferenceNumber={
index > 0 ? percentDifferenceNumber : 0
}
comparisonColorEnabled={comparisonColorEnabled}
>
{symbol}
</SymbolWrapper>
{index === 0
? prevNumber
: index === 1
? valueDifference
: percentDifferenceFormattedString}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]]

</ComparisonValue>
))}
</div>
</div>
</div>
Expand Down
Binary file modified ...t-frontend/plugins/plugin-chart-period-over-period-kpi/src/images/thumbnail.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
12 changes: 12 additions & 0 deletions superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/controlPanel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,18 @@ const config: ControlPanelConfig = {
},
},
],
[
{
name: 'comparison_color_enabled',
config: {
type: 'CheckboxControl',
label: t('Add color for positive/negative change'),
renderTrigger: true,
default: false,
description: t('Add color for positive/negative change'),
},
},
],
],
},
],
Expand Down
5 changes: 4 additions & 1 deletion superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/transformProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export default function transformProps(chartProps: ChartProps) {
yAxisFormat,
currencyFormat,
subheaderFontSize,
comparisonColorEnabled,
} = formData;
const { data: dataA = [] } = queriesData[0];
const { data: dataB = [] } = queriesData[1];
Expand Down Expand Up @@ -138,11 +139,13 @@ export default function transformProps(chartProps: ChartProps) {
bigNumber,
prevNumber,
valueDifference,
percentDifference,
percentDifferenceFormattedString: percentDifference,
boldText,
headerFontSize,
subheaderFontSize,
headerText,
compType,
comparisonColorEnabled,
percentDifferenceNumber: percentDifferenceNum,
};
}
15 changes: 11 additions & 4 deletions superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export interface PopKPIStylesProps {
headerFontSize: keyof typeof supersetTheme.typography.sizes;
subheaderFontSize: keyof typeof supersetTheme.typography.sizes;
boldText: boolean;
comparisonColorEnabled: boolean;
}

interface PopKPICustomizeProps {
Expand All @@ -39,6 +40,11 @@ export interface PopKPIComparisonValueStyleProps {
subheaderFontSize?: keyof typeof supersetTheme.typography.sizes;
}

export interface PopKPIComparisonSymbolStyleProps {
percentDifferenceNumber: number;
comparisonColorEnabled: boolean;
}

export type PopKPIQueryFormData = QueryFormData &
PopKPIStylesProps &
PopKPICustomizeProps;
Expand All @@ -48,9 +54,10 @@ export type PopKPIProps = PopKPIStylesProps &
data: TimeseriesDataRecord[];
metrics: Metric[];
metricName: String;
bigNumber: Number;
prevNumber: Number;
valueDifference: Number;
percentDifference: Number;
bigNumber: string;
prevNumber: string;
valueDifference: string;
percentDifferenceFormattedString: string;
compType: String;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also fix those capital S strings? 😛

percentDifferenceNumber: number;
};
Loading