From 3928113fe382b2f32019ffd41c9fd005bc23af27 Mon Sep 17 00:00:00 2001 From: Isaac Hellendag <2823852+hellendag@users.noreply.github.com> Date: Thu, 1 Aug 2024 14:07:20 -0500 Subject: [PATCH] [ui] Insights: Fix bar chart tooltip timezone (#23319) ## 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. --- .../ui-core/src/app/time/TimeContext.tsx | 2 +- .../ui-core/src/insights/InsightsBarChart.tsx | 11 +++- .../src/insights/InsightsBarChartTooltip.tsx | 23 +++++++- .../__stories__/InsightsBarChart.stories.tsx | 53 +++++++++++++++++++ .../src/schedules/TimestampDisplay.tsx | 8 +-- 5 files changed, 89 insertions(+), 8 deletions(-) diff --git a/js_modules/dagster-ui/packages/ui-core/src/app/time/TimeContext.tsx b/js_modules/dagster-ui/packages/ui-core/src/app/time/TimeContext.tsx index d9b0f055da600..2296978fe58cc 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/app/time/TimeContext.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/app/time/TimeContext.tsx @@ -6,7 +6,7 @@ import {useStateWithStorage} from '../../hooks/useStateWithStorage'; export const TimezoneStorageKey = 'TimezonePreference'; export const HourCycleKey = 'HourCyclePreference'; -type TimeContextValue = { +export type TimeContextValue = { timezone: ReturnType>; hourCycle: ReturnType>; }; diff --git a/js_modules/dagster-ui/packages/ui-core/src/insights/InsightsBarChart.tsx b/js_modules/dagster-ui/packages/ui-core/src/insights/InsightsBarChart.tsx index 844edd24e9370..2e9a1803af134 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/insights/InsightsBarChart.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/insights/InsightsBarChart.tsx @@ -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'; @@ -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'; @@ -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; @@ -115,6 +120,8 @@ export const InsightsBarChart = (props: Props) => { type={datapointType} label={label || ''} date={date} + timezone={timezone} + hourCycle={hourCycle} formattedValue={formattedValue} unitType={unitType} costMultiplier={costMultiplier} @@ -122,7 +129,7 @@ export const InsightsBarChart = (props: Props) => { /> ); }, - [costMultiplier, datapointType, metricLabel, unitType], + [datapointType, timezone, hourCycle, unitType, costMultiplier, metricLabel], ); const yAxis = useMemo(() => { diff --git a/js_modules/dagster-ui/packages/ui-core/src/insights/InsightsBarChartTooltip.tsx b/js_modules/dagster-ui/packages/ui-core/src/insights/InsightsBarChartTooltip.tsx index a9ebb0952b288..1b2e9d23941af 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/insights/InsightsBarChartTooltip.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/insights/InsightsBarChartTooltip.tsx @@ -5,6 +5,7 @@ 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 { @@ -12,6 +13,8 @@ interface Props { type: DatapointType; label: string; date: Date; + timezone: string; + hourCycle: HourCycle; formattedValue: string; unitType: ReportingUnitType; costMultiplier: number | null; @@ -19,7 +22,19 @@ interface Props { } 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 ( {
- +
{metricLabel}:{' '} diff --git a/js_modules/dagster-ui/packages/ui-core/src/insights/__stories__/InsightsBarChart.stories.tsx b/js_modules/dagster-ui/packages/ui-core/src/insights/__stories__/InsightsBarChart.stories.tsx index ae8291b951847..9eb009e46ccba 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/insights/__stories__/InsightsBarChart.stories.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/insights/__stories__/InsightsBarChart.stories.tsx @@ -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'; @@ -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 ( + +
+ +
+ + ); +}; + export const Empty = () => { const numDates = 10; diff --git a/js_modules/dagster-ui/packages/ui-core/src/schedules/TimestampDisplay.tsx b/js_modules/dagster-ui/packages/ui-core/src/schedules/TimestampDisplay.tsx index 25384afa9d12a..171ad7695e806 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/schedules/TimestampDisplay.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/schedules/TimestampDisplay.tsx @@ -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'; @@ -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; @@ -26,7 +28,7 @@ export const TimestampDisplay = (props: Props) => { locale, timezone: timezone || userTimezone, timeFormat, - hourCycle, + hourCycle: hourCycle || userHourCycle, }); return (