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.

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(Timeseries): Respect time grain #31765

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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 @@ -474,11 +474,11 @@ export default function transformProps(

const tooltipFormatter =
xAxisDataType === GenericDataType.Temporal
? getTooltipTimeFormatter(tooltipTimeFormat)
? getTooltipTimeFormatter(tooltipTimeFormat, timeGrainSqla)
: String;
const xAxisFormatter =
xAxisDataType === GenericDataType.Temporal
? getXAxisFormatter(xAxisTimeFormat)
? getXAxisFormatter(xAxisTimeFormat, timeGrainSqla)
: String;

const addYAxisTitleOffset = !!(yAxisTitle || yAxisTitleSecondary);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -444,11 +444,11 @@ export default function transformProps(

const tooltipFormatter =
xAxisDataType === GenericDataType.Temporal
? getTooltipTimeFormatter(tooltipTimeFormat)
? getTooltipTimeFormatter(tooltipTimeFormat, timeGrainSqla)
: String;
const xAxisFormatter =
xAxisDataType === GenericDataType.Temporal
? getXAxisFormatter(xAxisTimeFormat)
? getXAxisFormatter(xAxisTimeFormat, timeGrainSqla)
: String;

const {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
Ref,
} from 'react';

import { styled } from '@superset-ui/core';
import { styled, t } from '@superset-ui/core';
import { use, init, EChartsType } from 'echarts/core';
import {
SankeyChart,
Expand Down Expand Up @@ -126,7 +126,163 @@ function Echart(
useEffect(() => {
if (!divRef.current) return;
if (!chartRef.current) {
chartRef.current = init(divRef.current);
chartRef.current = init(divRef.current, null, {
locale: {
time: {
month: [
t('January'),
t('February'),
t('March'),
t('April'),
t('May'),
t('June'),
t('July'),
t('August'),
t('September'),
t('October'),
t('November'),
t('December'),
],
monthAbbr: [
t('Jan'),
t('Feb'),
t('Mar'),
t('Apr'),
t('May'),
t('Jun'),
t('Jul'),
t('Aug'),
t('Sep'),
t('Oct'),
t('Nov'),
t('Dec'),
],
dayOfWeek: [
t('Sunday'),
t('Monday'),
t('Tuesday'),
t('Wednesday'),
t('Thursday'),
t('Friday'),
t('Saturday'),
],
dayOfWeekAbbr: [
t('Sun'),
t('Mon'),
t('Tue'),
t('Wed'),
t('Thu'),
t('Fri'),
t('Sat'),
],
},
legend: {
selector: {
all: t('All'),
inverse: t('Inv'),
},
},
toolbox: {
brush: {
title: {
rect: t('Box Select'),
polygon: t('Lasso Select'),
lineX: t('Horizontally Select'),
lineY: t('Vertically Select'),
keep: t('Keep Selections'),
clear: t('Clear Selections'),
},
},
dataView: {
title: t('Data View'),
lang: [t('Data View'), t('Close'), 'Refresh'],
},

This comment was marked as resolved.

dataZoom: {
title: {
zoom: t('Zoom'),
back: t('Zoom Reset'),
},
},
magicType: {
title: {
line: t('Switch to Line Chart'),
bar: t('Switch to Bar Chart'),
stack: t('Stack'),
tiled: t('Tile'),
},
},
restore: {
title: t('Restore'),
},
saveAsImage: {
title: t('Save as Image'),
lang: ['Right Click to Save Image'],
},
},
series: {
typeNames: {
pie: t('Pie chart'),
bar: t('Bar chart'),
line: t('Line chart'),
scatter: t('Scatter plot'),
effectScatter: t('Ripple scatter plot'),
radar: t('Radar chart'),
tree: t('Tree'),
treemap: t('Treemap'),
boxplot: t('Boxplot'),
candlestick: t('Candlestick'),
k: t('K line chart'),
heatmap: t('Heat map'),
map: t('Map'),
parallel: t('Parallel coordinate map'),
lines: t('Line graph'),
graph: t('Relationship graph'),
sankey: t('Sankey diagram'),
funnel: t('Funnel chart'),
gauge: t('Gauge'),
pictorialBar: t('Pictorial bar'),
themeRiver: t('Theme River Map'),
sunburst: t('Sunburst'),
custom: t('Custom chart'),
chart: t('Chart'),
},
},
aria: {
general: {
withTitle: t('This is a chart about "{title}"'),
withoutTitle: t('This is a chart'),
},
series: {
single: {
prefix: '',
withName: t(' with type {seriesType} named {seriesName}.'),
withoutName: t(' with type {seriesType}.'),
},
multiple: {
prefix: t('. It consists of {seriesCount} series count.'),
withName: t(
' The {seriesId} series is a {seriesType} representing {seriesName}.',
),
withoutName: t(' The {seriesId} series is a {seriesType}.'),
separator: {
middle: '',
end: '',
},
},
},
data: {
allData: t('The data is as follows: '),
partialData: t('The first {displayCnt} items are: '),
withName: t('the data for {name} is {value}'),
withoutName: '{value}',
separator: {
middle: ', ',
end: '. ',
},
},
},
},
});
}

Object.entries(eventHandlers || {}).forEach(([name, handler]) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
SMART_DATE_ID,
SMART_DATE_VERBOSE_ID,
TimeFormatter,
TimeGranularity,
ValueFormatter,
} from '@superset-ui/core';

Expand Down Expand Up @@ -76,24 +77,40 @@ export const getYAxisFormatter = (

export function getTooltipTimeFormatter(
format?: string,
timeGrain?: TimeGranularity,
): TimeFormatter | StringConstructor {
if (
timeGrain === TimeGranularity.QUARTER ||
timeGrain === TimeGranularity.MONTH ||
timeGrain === TimeGranularity.YEAR
) {
return getTimeFormatter(undefined, timeGrain);
}
if (format === SMART_DATE_ID) {
return getSmartDateDetailedFormatter();
return getTimeFormatter(SMART_DATE_DETAILED_ID, timeGrain);
}
Comment on lines 89 to 91
Copy link

Choose a reason for hiding this comment

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

Incorrect Override of Smart Date Format category Functionality

Tell me more
What is the issue?

When SMART_DATE_ID format is specified, the code overrides it with SMART_DATE_DETAILED_ID, which contradicts the user's explicit format choice.

Why this matters

Users selecting SMART_DATE_ID format will unexpectedly receive a different formatting style than requested, potentially causing confusion and inconsistency in the application's behavior.

Suggested change

Respect the user's format choice by modifying the code to:

if (format === SMART_DATE_ID) {
    return getTimeFormatter(SMART_DATE_ID, timeGrain);
}
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

if (format) {
return getTimeFormatter(format);
return getTimeFormatter(format, timeGrain);
}
return String;
}

export function getXAxisFormatter(
format?: string,
timeGrain?: TimeGranularity,
): TimeFormatter | StringConstructor | undefined {
if (
timeGrain === TimeGranularity.QUARTER ||
timeGrain === TimeGranularity.MONTH ||
timeGrain === TimeGranularity.YEAR
) {
return getTimeFormatter(undefined, timeGrain);
}
if (format === SMART_DATE_ID || !format) {
return undefined;
}
if (format) {
return getTimeFormatter(format);
return getTimeFormatter(format, timeGrain);
}
return String;
}
Loading