-
Notifications
You must be signed in to change notification settings - Fork 15
Refactor date range picker #1225
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
17736e6
date picker uses local state instead of formik
david-crespo 2685a4f
change from hook to normal component, fix almost all the tests
david-crespo 197c0ca
get validation working and fix the test for that
david-crespo 1fb56a5
update system and silo metrics pages even though they're off
david-crespo af8bf0b
make the preset listbox a little smaller
david-crespo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
148 changes: 148 additions & 0 deletions
148
app/components/form/fields/DateTimeRangePicker.spec.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,148 @@ | ||
| import { fireEvent, render, screen } from '@testing-library/react' | ||
| import { subDays, subHours } from 'date-fns' | ||
| import { vi } from 'vitest' | ||
|
|
||
| import { clickByRole } from 'app/test/unit' | ||
|
|
||
| import type { RangeKey } from './DateTimeRangePicker' | ||
| import { DateTimeRangePicker, dateForInput } from './DateTimeRangePicker' | ||
|
|
||
| const now = new Date(2020, 1, 1) | ||
|
|
||
| function renderLastDay() { | ||
| const onChange = vi.fn() | ||
| render( | ||
| <DateTimeRangePicker | ||
| initialPreset="lastDay" | ||
| startTime={subDays(now, 1)} | ||
| endTime={now} | ||
| onChange={onChange} | ||
| /> | ||
| ) | ||
| return { onChange } | ||
| } | ||
|
|
||
| describe('useDateTimeRangePicker', () => { | ||
| it.each([ | ||
| ['lastHour', subHours(now, 1)], | ||
| ['last3Hours', subHours(now, 3)], | ||
| ['lastDay', subDays(now, 1)], | ||
| ['lastWeek', subDays(now, 7)], | ||
| ['last30Days', subDays(now, 30)], | ||
| ])('sets initial start and end', (preset, start) => { | ||
| const onChange = vi.fn() | ||
| render( | ||
| <DateTimeRangePicker | ||
| initialPreset={preset as RangeKey} | ||
| startTime={start} | ||
| endTime={now} | ||
| onChange={onChange} | ||
| /> | ||
| ) | ||
|
|
||
| expect(screen.getByLabelText('Start time')).toHaveValue(dateForInput(start)) | ||
| expect(screen.getByLabelText('End time')).toHaveValue(dateForInput(now)) | ||
| }) | ||
| }) | ||
|
|
||
| it.each([ | ||
| ['Last hour', subHours(now, 1)], | ||
| ['Last 3 hours', subHours(now, 3)], | ||
| // ['Last day', subDays(now, 1)], // skip because we're starting on it | ||
| ['Last week', subDays(now, 7)], | ||
| ['Last 30 days', subDays(now, 30)], | ||
| ])('choosing a preset sets the times', (option, start) => { | ||
| vi.useFakeTimers() | ||
| vi.setSystemTime(now) | ||
|
|
||
| const { onChange } = renderLastDay() | ||
|
|
||
| clickByRole('button', 'Choose a time range') | ||
| clickByRole('option', option) | ||
|
|
||
| expect(onChange).toBeCalledWith(start, now) | ||
|
|
||
| vi.useRealTimers() | ||
| }) | ||
|
|
||
| describe('custom mode', () => { | ||
| it('enables datetime inputs', () => { | ||
| const { onChange } = renderLastDay() | ||
|
|
||
| expect(screen.getByLabelText('Start time')).toBeDisabled() | ||
|
|
||
| clickByRole('button', 'Choose a time range') | ||
| clickByRole('option', 'Custom...') | ||
|
|
||
| expect(onChange).not.toBeCalled() | ||
| expect(screen.getByLabelText('Start time')).toBeEnabled() | ||
| expect(screen.getByRole('button', { name: 'Reset' })).toBeDisabled() | ||
| expect(screen.getByRole('button', { name: 'Load' })).toBeDisabled() | ||
| }) | ||
|
|
||
| it('clicking load after changing date changes range', async () => { | ||
| const { onChange } = renderLastDay() | ||
|
|
||
| expect(screen.getByLabelText('Start time')).toHaveValue(dateForInput(subDays(now, 1))) | ||
| expect(screen.getByLabelText('End time')).toHaveValue(dateForInput(now)) | ||
|
|
||
| clickByRole('button', 'Choose a time range') | ||
| clickByRole('option', 'Custom...') | ||
|
|
||
| // change input values. figuring out how to actually interact with the | ||
| // input through clicks and typing is too complicated | ||
| const startInput = screen.getByLabelText('Start time') | ||
| fireEvent.change(startInput, { target: { value: '2020-01-15T00:00' } }) | ||
|
|
||
| const endInput = screen.getByLabelText('End time') | ||
| fireEvent.change(endInput, { target: { value: '2020-01-17T00:00' } }) | ||
|
|
||
| // changing the input value without clicking Load doesn't do anything | ||
| expect(onChange).not.toBeCalled() | ||
|
|
||
| // clicking Load calls onChange | ||
| clickByRole('button', 'Load') | ||
| expect(onChange).toBeCalledWith(new Date(2020, 0, 15), new Date(2020, 0, 17)) | ||
| }) | ||
|
|
||
| it('clicking reset after changing inputs resets inputs', async () => { | ||
| const { onChange } = renderLastDay() | ||
|
|
||
| expect(screen.getByLabelText('Start time')).toHaveValue(dateForInput(subDays(now, 1))) | ||
| expect(screen.getByLabelText('End time')).toHaveValue(dateForInput(now)) | ||
|
|
||
| clickByRole('button', 'Choose a time range') | ||
| clickByRole('option', 'Custom...') | ||
|
|
||
| const startInput = screen.getByLabelText('Start time') | ||
| fireEvent.change(startInput, { target: { value: '2020-01-15T00:00' } }) | ||
| expect(startInput).toHaveValue('2020-01-15T00:00') | ||
|
|
||
| const endInput = screen.getByLabelText('End time') | ||
| fireEvent.change(endInput, { target: { value: '2020-01-17T00:00' } }) | ||
| expect(endInput).toHaveValue('2020-01-17T00:00') | ||
|
|
||
| // clicking reset resets the inputs | ||
| clickByRole('button', 'Reset') | ||
| expect(startInput).toHaveValue('2020-01-31T00:00') | ||
| expect(endInput).toHaveValue('2020-02-01T00:00') | ||
|
|
||
| // onChange is never called | ||
| expect(onChange).not.toBeCalled() | ||
| }) | ||
|
|
||
| it('shows error for invalid range', () => { | ||
| renderLastDay() | ||
|
|
||
| clickByRole('button', 'Choose a time range') | ||
| clickByRole('option', 'Custom...') | ||
|
|
||
| const startInput = screen.getByLabelText('Start time') | ||
| expect(startInput).toHaveValue('2020-01-31T00:00') | ||
|
|
||
| // start date is after end | ||
| fireEvent.change(startInput, { target: { value: '2020-02-03T00:00' } }) | ||
|
|
||
| screen.getByText('Start time must be earlier than end time') | ||
| }) | ||
| }) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,180 @@ | ||
| import { format, subDays, subHours } from 'date-fns' | ||
| import { useMemo, useState } from 'react' | ||
|
|
||
| import { Listbox, useInterval } from '@oxide/ui' | ||
| import { Button, TextInput } from '@oxide/ui' | ||
|
|
||
| export const dateForInput = (d: Date) => format(d, "yyyy-MM-dd'T'HH:mm") | ||
|
|
||
| const rangePresets = [ | ||
| { label: 'Last hour', value: 'lastHour' as const }, | ||
| { label: 'Last 3 hours', value: 'last3Hours' as const }, | ||
| { label: 'Last day', value: 'lastDay' as const }, | ||
| { label: 'Last week', value: 'lastWeek' as const }, | ||
| { label: 'Last 30 days', value: 'last30Days' as const }, | ||
| { label: 'Custom...', value: 'custom' as const }, | ||
| ] | ||
|
|
||
| // custom doesn't have an associated range | ||
| type RangeKeyAll = typeof rangePresets[number]['value'] | ||
| export type RangeKey = Exclude<RangeKeyAll, 'custom'> | ||
|
|
||
| // Record ensures we have an entry for every preset | ||
| const computeStart: Record<RangeKey, (now: Date) => Date> = { | ||
| lastHour: (now) => subHours(now, 1), | ||
| last3Hours: (now) => subHours(now, 3), | ||
| lastDay: (now) => subDays(now, 1), | ||
| lastWeek: (now) => subDays(now, 7), | ||
| last30Days: (now) => subDays(now, 30), | ||
| } | ||
|
|
||
| // Limitations: | ||
| // - list of presets is hard-coded | ||
| // - initial preset can't be "custom" | ||
|
|
||
| type Props = { | ||
| initialPreset: RangeKey | ||
| /** | ||
| * if set and range is a relative preset, update the range to have `endTime` | ||
| * of now every X ms | ||
| */ | ||
| slideInterval?: number | ||
| startTime: Date | ||
| endTime: Date | ||
| onChange: (startTime: Date, endTime: Date) => void | ||
| } | ||
|
|
||
| export function useDateTimeRangePickerState(initialPreset: RangeKey) { | ||
| // default endTime is now, i.e., mount time | ||
| const now = useMemo(() => new Date(), []) | ||
|
|
||
| const [startTime, setStartTime] = useState(computeStart[initialPreset](now)) | ||
| const [endTime, setEndTime] = useState(now) | ||
|
|
||
| function onChange(newStart: Date, newEnd: Date) { | ||
| setStartTime(newStart) | ||
| setEndTime(newEnd) | ||
| } | ||
|
|
||
| return { startTime, endTime, onChange } | ||
| } | ||
|
|
||
| function validateRange(startTime: Date, endTime: Date): string | null { | ||
| if (startTime >= endTime) { | ||
| return 'Start time must be earlier than end time' | ||
| } | ||
|
|
||
| return null | ||
| } | ||
|
|
||
| /** | ||
| * Exposes `startTime` and `endTime` plus the whole set of picker UI controls as | ||
| * a JSX element to render. | ||
| */ | ||
| export function DateTimeRangePicker({ | ||
| initialPreset, | ||
| slideInterval, | ||
| startTime, | ||
| endTime, | ||
| onChange, | ||
| }: Props) { | ||
| const [preset, setPreset] = useState<RangeKeyAll>(initialPreset) | ||
|
|
||
| // needs a separate pair of values because they can be edited without | ||
| // submitting and updating the graphs | ||
| const [startTimeInput, setStartTimeInput] = useState(startTime) | ||
| const [endTimeInput, setEndTimeInput] = useState(endTime) | ||
|
|
||
| // TODO: validate inputs on change and display error someplace | ||
| const error = validateRange(startTimeInput, endTimeInput) | ||
|
|
||
| const customInputsDirty = startTime !== startTimeInput || endTime !== endTimeInput | ||
|
|
||
| const enableInputs = preset === 'custom' | ||
|
|
||
| /** Set the input values and call the passed-on onChange with the new times */ | ||
| function setTimesForPreset(newPreset: RangeKey) { | ||
| const now = new Date() | ||
| const newStartTime = computeStart[newPreset](now) | ||
| onChange(newStartTime, now) | ||
| setStartTimeInput(newStartTime) | ||
| setEndTimeInput(now) | ||
| } | ||
|
|
||
| useInterval( | ||
| () => { | ||
| if (preset !== 'custom') setTimesForPreset(preset) | ||
| }, | ||
| slideInterval && preset !== 'custom' ? slideInterval : null | ||
| ) | ||
|
|
||
| return ( | ||
| <form className="flex h-20 gap-4"> | ||
| <Listbox | ||
| className="mr-4 w-48" // in addition to gap-4 | ||
| name="preset" | ||
| defaultValue={initialPreset} | ||
| aria-label="Choose a time range" | ||
| items={rangePresets} | ||
| onChange={(item) => { | ||
| if (item) { | ||
| setPreset(item.value as RangeKeyAll) | ||
| if (item.value !== 'custom') setTimesForPreset(item.value as RangeKey) | ||
| } | ||
| }} | ||
| /> | ||
|
|
||
| {/* TODO: real React date picker lib instead of native for consistent styling across browsers */} | ||
| <div> | ||
| <div className="flex gap-4"> | ||
| <TextInput | ||
| id="startTime" | ||
| type="datetime-local" | ||
| className="h-10" | ||
| // TODO: figure out error | ||
| error={false} | ||
| aria-label="Start time" | ||
| disabled={!enableInputs} | ||
| required | ||
| value={dateForInput(startTimeInput)} | ||
| onChange={(e) => setStartTimeInput(new Date(e.target.value))} | ||
| /> | ||
| <TextInput | ||
| id="endTime" | ||
| type="datetime-local" | ||
| className="h-10" | ||
| // TODO: figure out error | ||
| error={false} | ||
| aria-label="End time" | ||
| disabled={!enableInputs} | ||
| required | ||
| value={dateForInput(endTimeInput)} | ||
| onChange={(e) => setEndTimeInput(new Date(e.target.value))} | ||
| /> | ||
| </div> | ||
| {error && <div className="mt-2 text-center text-error">{error}</div>} | ||
| </div> | ||
| {/* TODO: fix goofy ass button text. use icons? tooltips to explain? lord */} | ||
| {enableInputs && ( | ||
| <Button | ||
| disabled={!customInputsDirty} | ||
| // reset inputs back to whatever they were | ||
| onClick={() => { | ||
| setStartTimeInput(startTime) | ||
| setEndTimeInput(endTime) | ||
| }} | ||
| > | ||
| Reset | ||
| </Button> | ||
| )} | ||
| {enableInputs && ( | ||
| <Button | ||
| disabled={!customInputsDirty || !!error} | ||
| onClick={() => onChange(startTimeInput, endTimeInput)} | ||
| > | ||
| Load | ||
| </Button> | ||
| )} | ||
| </form> | ||
| ) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weirdly, when I had the
vi.setSystemTimein abeforeAllor abeforeEach, it did not work. Something to remember, possibly a bug to report.