Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

fix: bignumber format by time formatter #1307

Merged
merged 3 commits into from
Sep 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ type BigNumberVisProps = {
height: number;
bigNumber?: number | null;
bigNumberFallback?: TimeSeriesDatum;
formatNumber: NumberFormatter;
headerFormatter: NumberFormatter | TimeFormatter;
formatTime: TimeFormatter;
fromDatetime?: number;
toDatetime?: number;
Expand All @@ -98,7 +98,7 @@ class BigNumberVis extends React.PureComponent<BigNumberVisProps, {}> {

static defaultProps = {
className: '',
formatNumber: defaultNumberFormatter,
headerFormatter: defaultNumberFormatter,
formatTime: smartDateVerboseFormatter,
headerFontSize: PROPORTION.HEADER,
kickerFontSize: PROPORTION.KICKER,
Expand Down Expand Up @@ -173,8 +173,8 @@ class BigNumberVis extends React.PureComponent<BigNumberVisProps, {}> {
}

renderHeader(maxHeight: number) {
const { bigNumber, formatNumber, width } = this.props;
const text = bigNumber === null ? t('No data') : formatNumber(bigNumber);
const { bigNumber, headerFormatter, width } = this.props;
const text = bigNumber === null ? t('No data') : headerFormatter(bigNumber);

const container = this.createTemporaryContainer();
document.body.append(container);
Expand Down Expand Up @@ -246,7 +246,7 @@ class BigNumberVis extends React.PureComponent<BigNumberVisProps, {}> {
mainColor,
subheader,
startYAxisAtZero,
formatNumber,
headerFormatter,
formatTime,
fromDatetime,
timeRangeFixed,
Expand Down Expand Up @@ -283,7 +283,8 @@ class BigNumberVis extends React.PureComponent<BigNumberVisProps, {}> {
<XYChart
snapTooltipToDataX
ariaLabel={`Big number visualization ${subheader}`}
renderTooltip={renderTooltipFactory(formatTime, formatNumber)}
// headerFormatter always NumberFormatter in BigNumber with treadline
renderTooltip={renderTooltipFactory(formatTime, headerFormatter as NumberFormatter)}
xScale={xScale}
yScale={{
type: 'linear',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ export default function transformProps(chartProps: BigNumberChartProps) {
} = formData;
const granularity = extractTimegrain(rawFormData as QueryFormData);
let { yAxisFormat } = formData;
const { headerFormatSelector, headerTimestampFormat } = formData;
const { data = [], from_dttm: fromDatetime, to_dttm: toDatetime } = queriesData[0];
const metricName = typeof metric === 'string' ? metric : metric.label;
const compareLag = Number(compareLag_) || 0;
Expand Down Expand Up @@ -146,7 +147,9 @@ export default function transformProps(chartProps: BigNumberChartProps) {
});
}

const formatNumber = getNumberFormatter(yAxisFormat);
const headerFormatter = headerFormatSelector
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename headerFormatSelector to isFormatTimestamp or something like that? It's not obvious that it's a boolean value which is true if formatter is for timestamp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will push a separate PR after this. Thanks!

? getTimeFormatter(headerTimestampFormat)
: getNumberFormatter(yAxisFormat);
const formatTime =
timeFormat === smartDateFormatter.id
? getTimeFormatterForGranularity(granularity)
Expand All @@ -158,7 +161,7 @@ export default function transformProps(chartProps: BigNumberChartProps) {
bigNumber,
bigNumberFallback,
className,
formatNumber,
headerFormatter,
formatTime,
headerFontSize,
subheaderFontSize,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,12 @@
* under the License.
*/
import { t } from '@superset-ui/core';
import { ControlPanelConfig, sections } from '@superset-ui/chart-controls';
import {
ControlPanelConfig,
D3_FORMAT_DOCS,
D3_TIME_FORMAT_OPTIONS,
sections,
} from '@superset-ui/chart-controls';
import { headerFontSize, subheaderFontSize } from '../sharedControls';

export default {
Expand All @@ -43,6 +48,36 @@ export default {
},
],
['y_axis_format'],
[
{
name: 'header_format_selector',
config: {
type: 'CheckboxControl',
label: t('Timestamp Format'),
renderTrigger: true,
default: false,
description: t('Whether to format the timestamp'),
},
},
],
[
{
name: 'header_timestamp_format',
config: {
type: 'SelectControl',
freeForm: true,
label: t('Date format'),
renderTrigger: true,
choices: D3_TIME_FORMAT_OPTIONS,
default: '%d-%m-%Y %H:%M:%S',
description: D3_FORMAT_DOCS,
visibility(props) {
const { header_format_selector } = props.form_data;
return !!header_format_selector;
},
},
},
],
],
},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ describe('BigNumber', () => {
},
};
const transformed = transformProps(propsWithDatasource);
expect(transformed.formatNumber(transformed.bigNumber)).toStrictEqual('1.23');
expect(transformed.headerFormatter(transformed.bigNumber)).toStrictEqual('1.23');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for timestamp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will push a separate PR after this. Thanks!

});
});
});