Skip to content

Commit

Permalink
[ui] Insights: Fix bar chart tooltip timezone (#23319)
Browse files Browse the repository at this point in the history
## Summary & Motivation

Fixing a bug I overlooked on Insights bar charts for asset metadata: the
tooltip timezone and hour-cycle format are incorrect, as they are based
on the default browser values instead of the user's settings. Fix this
by passing the values through to the tooltip, which is rendered without
the time context available.

## How I Tested These Changes

Storybook example with an arbitrary timezone chosen. Verify on hover
that the timezone and hour-cycle format are correct, matching the x axis
of the chart.
  • Loading branch information
hellendag authored Aug 1, 2024
1 parent bcd0b86 commit 3928113
Show file tree
Hide file tree
Showing 5 changed files with 89 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {useStateWithStorage} from '../../hooks/useStateWithStorage';
export const TimezoneStorageKey = 'TimezonePreference';
export const HourCycleKey = 'HourCyclePreference';

type TimeContextValue = {
export type TimeContextValue = {
timezone: ReturnType<typeof useStateWithStorage<string>>;
hourCycle: ReturnType<typeof useStateWithStorage<HourCycle>>;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
Tooltip,
} from 'chart.js';
import * as React from 'react';
import {useCallback, useMemo} from 'react';
import {useCallback, useContext, useMemo} from 'react';
import {Bar} from 'react-chartjs-2';

import {iconNameForMetric} from './IconForMetricName';
Expand All @@ -19,6 +19,7 @@ import {EmptyStateContainer, LoadingStateContainer} from './InsightsChartShared'
import {formatMetric} from './formatMetric';
import {RenderTooltipFn, renderInsightsChartTooltip} from './renderInsightsChartTooltip';
import {BarDatapoint, BarValue, DatapointType, ReportingUnitType} from './types';
import {TimeContext} from '../app/time/TimeContext';
import {useRGBColorsForTheme} from '../app/useRGBColorsForTheme';
import {useFormatDateTime} from '../ui/useFormatDateTime';

Expand Down Expand Up @@ -71,6 +72,10 @@ export const InsightsBarChart = (props: Props) => {
}, [values]);

const formatDateTime = useFormatDateTime();
const {
timezone: [timezone],
hourCycle: [hourCycle],
} = useContext(TimeContext);

// Don't show the y axis while loading datapoints, to avoid jumping renders.
const showYAxis = values.length > 0;
Expand Down Expand Up @@ -115,14 +120,16 @@ export const InsightsBarChart = (props: Props) => {
type={datapointType}
label={label || ''}
date={date}
timezone={timezone}
hourCycle={hourCycle}
formattedValue={formattedValue}
unitType={unitType}
costMultiplier={costMultiplier}
metricLabel={metricLabel}
/>
);
},
[costMultiplier, datapointType, metricLabel, unitType],
[datapointType, timezone, hourCycle, unitType, costMultiplier, metricLabel],
);

const yAxis = useMemo(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,36 @@ import {InsightsIdentifierDot} from './InsightsIdentifierDot';
import {TOTAL_COST_FORMATTER} from './costFormatters';
import {formatMetric, stripFormattingFromNumber} from './formatMetric';
import {DatapointType, ReportingUnitType} from './types';
import {HourCycle} from '../app/time/HourCycle';
import {TimestampDisplay} from '../schedules/TimestampDisplay';

interface Props {
color: string;
type: DatapointType;
label: string;
date: Date;
timezone: string;
hourCycle: HourCycle;
formattedValue: string;
unitType: ReportingUnitType;
costMultiplier: number | null;
metricLabel: string;
}

export const InsightsBarChartTooltip = (props: Props) => {
const {color, type, label, date, formattedValue, unitType, metricLabel, costMultiplier} = props;
const {
color,
type,
label,
date,
timezone,
hourCycle,
formattedValue,
unitType,
metricLabel,
costMultiplier,
} = props;

return (
<TooltipCard>
<Box
Expand All @@ -35,7 +50,11 @@ export const InsightsBarChartTooltip = (props: Props) => {
</Box>
<Box padding={{vertical: 8, horizontal: 12}} flex={{direction: 'column', gap: 4}}>
<div style={{fontWeight: 600, fontSize: '12px', color: Colors.textLight()}}>
<TimestampDisplay timestamp={date.getTime() / 1000} />
<TimestampDisplay
timestamp={date.getTime() / 1000}
timezone={timezone}
hourCycle={hourCycle}
/>
</div>
<div style={{fontSize: '12px', color: Colors.textLight()}}>
{metricLabel}:{' '}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {faker} from '@faker-js/faker';
import {Meta} from '@storybook/react';
import {useEffect, useMemo, useState} from 'react';

import {TimeContext, TimeContextValue} from '../../app/time/TimeContext';
import {InsightsBarChart} from '../InsightsBarChart';
import {ReportingUnitType} from '../types';

Expand Down Expand Up @@ -64,6 +65,58 @@ export const Default = () => {
);
};

export const ArbitraryTimezone = () => {
const numDates = 50;

const datapoint = useMemo(() => {
const barValues = new Array(numDates).fill(null).map(() => {
const key = faker.random.alphaNumeric(8);
return {
value: randomDataPoint(MIN, MAX),
key,
href: `/runs/${key}`,
label: `Run ${key}`,
};
});

return {
barColor: Colors.dataVizBlurple(),
type: 'asset-group' as const,
label: faker.random.words(3).replaceAll(' ', '-').toLowerCase(),
values: barValues,
};
}, []);

const timestamps = useMemo(() => {
return new Array(numDates).fill(null).map((_, ii) => JUNE_1_2023_EDT + TWO_HOURS * ii);
}, [numDates]);

const timeContext: TimeContextValue = useMemo(
() => ({
timezone: ['Asia/Kolkata', () => 'Asia/Kolkata', () => {}],
hourCycle: ['h23', () => 'h23', () => {}],
}),
[],
);

return (
<TimeContext.Provider value={timeContext}>
<div style={{height: '600px'}}>
<InsightsBarChart
datapointType="asset-group"
datapoint={datapoint}
loading={false}
metricLabel="Dagster credits"
metricName="__dagster_dagster_credits"
unitType={ReportingUnitType.INTEGER}
timestamps={timestamps}
costMultiplier={null}
/>
</div>
</TimeContext.Provider>
);
};

export const Empty = () => {
const numDates = 10;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {Colors, Icon, Tooltip} from '@dagster-io/ui-components';
import {useContext} from 'react';
import styled from 'styled-components';

import {HourCycle} from '../app/time/HourCycle';
import {TimeContext} from '../app/time/TimeContext';
import {DEFAULT_TIME_FORMAT, TimeFormat} from '../app/time/TimestampFormat';
import {timestampToString} from '../app/time/timestampToString';
Expand All @@ -10,14 +11,15 @@ interface Props {
timestamp: number;
timezone?: string | null;
timeFormat?: TimeFormat;
hourCycle?: HourCycle | null;
tooltipTimeFormat?: TimeFormat;
}

export const TimestampDisplay = (props: Props) => {
const {timestamp, timezone, timeFormat, tooltipTimeFormat} = props;
const {timestamp, timezone, timeFormat, hourCycle, tooltipTimeFormat} = props;
const {
timezone: [userTimezone],
hourCycle: [hourCycle],
hourCycle: [userHourCycle],
} = useContext(TimeContext);

const locale = navigator.language;
Expand All @@ -26,7 +28,7 @@ export const TimestampDisplay = (props: Props) => {
locale,
timezone: timezone || userTimezone,
timeFormat,
hourCycle,
hourCycle: hourCycle || userHourCycle,
});

return (
Expand Down

1 comment on commit 3928113

@github-actions
Copy link

Choose a reason for hiding this comment

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

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-dvyfn8bfu-elementl.vercel.app

Built with commit 3928113.
This pull request is being automatically deployed with vercel-action

Please sign in to comment.