-
Notifications
You must be signed in to change notification settings - Fork 897
fix(site): fix floating number on duration fields #13209
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 15 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
6d359fd
Add DurationField component
BrunoQuaresma 5ac0602
Add empty story
BrunoQuaresma 95e4f44
Avoid negative values
BrunoQuaresma 4a48102
Use duration field time_til_dormant_ms
BrunoQuaresma 09ddb8f
Fix parent updates
BrunoQuaresma 3427160
Fix helper text
BrunoQuaresma eef159c
Support errors
BrunoQuaresma b3042c6
Use valueMs to make the value is in miliseconds
BrunoQuaresma e6101cf
Replace useState by useReducer
BrunoQuaresma c82fc8f
Add 10 to base int
BrunoQuaresma 1b77488
Merge branch 'main' of https://github.com/coder/coder into bq/fix-flo…
BrunoQuaresma 9036465
Use a number mask
BrunoQuaresma cac3a89
Make number input full width
BrunoQuaresma d89ec94
Add duration field to auto deletion
BrunoQuaresma 0a93c9f
Apply duration field to failure clean up
BrunoQuaresma 5a35e1a
Merge branch 'main' of https://github.com/coder/coder into bq/fix-flo…
BrunoQuaresma 45a5eb5
Add extra tests to storybook
BrunoQuaresma a6ff426
Fix tests
BrunoQuaresma 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
44 changes: 44 additions & 0 deletions
44
site/src/components/DurationField/DurationField.stories.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,44 @@ | ||
import type { Meta, StoryObj } from "@storybook/react"; | ||
import { useState } from "react"; | ||
import { DurationField } from "./DurationField"; | ||
|
||
const meta: Meta<typeof DurationField> = { | ||
title: "components/DurationField", | ||
component: DurationField, | ||
args: { | ||
label: "Duration", | ||
}, | ||
render: function RenderComponent(args) { | ||
const [value, setValue] = useState<number>(args.valueMs); | ||
return ( | ||
<DurationField | ||
{...args} | ||
valueMs={value} | ||
onChange={(value) => setValue(value)} | ||
/> | ||
); | ||
}, | ||
}; | ||
|
||
export default meta; | ||
type Story = StoryObj<typeof DurationField>; | ||
|
||
export const Hours: Story = { | ||
args: { | ||
valueMs: hoursToMs(16), | ||
}, | ||
}; | ||
|
||
export const Days: Story = { | ||
args: { | ||
valueMs: daysToMs(2), | ||
}, | ||
}; | ||
|
||
function hoursToMs(hours: number): number { | ||
return hours * 60 * 60 * 1000; | ||
} | ||
|
||
function daysToMs(days: number): number { | ||
return days * 24 * 60 * 60 * 1000; | ||
} |
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,187 @@ | ||
import KeyboardArrowDown from "@mui/icons-material/KeyboardArrowDown"; | ||
import FormHelperText from "@mui/material/FormHelperText"; | ||
import MenuItem from "@mui/material/MenuItem"; | ||
import Select from "@mui/material/Select"; | ||
import TextField, { type TextFieldProps } from "@mui/material/TextField"; | ||
import { type FC, useEffect, useReducer } from "react"; | ||
import { | ||
type TimeUnit, | ||
durationInDays, | ||
durationInHours, | ||
suggestedTimeUnit, | ||
} from "utils/time"; | ||
|
||
type DurationFieldProps = Omit<TextFieldProps, "value" | "onChange"> & { | ||
valueMs: number; | ||
onChange: (value: number) => void; | ||
}; | ||
|
||
type State = { | ||
unit: TimeUnit; | ||
// Handling empty values as strings in the input simplifies the process, | ||
// especially when a user clears the input field. | ||
durationFieldValue: string; | ||
}; | ||
|
||
type Action = | ||
| { type: "SYNC_WITH_PARENT"; parentValueMs: number } | ||
| { type: "CHANGE_DURATION_FIELD_VALUE"; fieldValue: string } | ||
| { type: "CHANGE_TIME_UNIT"; unit: TimeUnit }; | ||
|
||
const reducer = (state: State, action: Action): State => { | ||
switch (action.type) { | ||
case "SYNC_WITH_PARENT": { | ||
return initState(action.parentValueMs); | ||
} | ||
case "CHANGE_DURATION_FIELD_VALUE": { | ||
return { | ||
...state, | ||
durationFieldValue: action.fieldValue, | ||
}; | ||
} | ||
case "CHANGE_TIME_UNIT": { | ||
const currentDurationMs = durationInMs( | ||
state.durationFieldValue, | ||
state.unit, | ||
); | ||
|
||
if ( | ||
action.unit === "days" && | ||
!canConvertDurationToDays(currentDurationMs) | ||
) { | ||
return state; | ||
} | ||
|
||
return { | ||
unit: action.unit, | ||
durationFieldValue: | ||
action.unit === "hours" | ||
? durationInHours(currentDurationMs).toString() | ||
: durationInDays(currentDurationMs).toString(), | ||
}; | ||
} | ||
default: { | ||
return state; | ||
} | ||
} | ||
}; | ||
|
||
export const DurationField: FC<DurationFieldProps> = (props) => { | ||
const { | ||
valueMs: parentValueMs, | ||
onChange, | ||
helperText, | ||
...textFieldProps | ||
} = props; | ||
const [state, dispatch] = useReducer(reducer, initState(parentValueMs)); | ||
const currentDurationMs = durationInMs(state.durationFieldValue, state.unit); | ||
|
||
useEffect(() => { | ||
if (parentValueMs !== currentDurationMs) { | ||
dispatch({ type: "SYNC_WITH_PARENT", parentValueMs }); | ||
} | ||
}, [currentDurationMs, parentValueMs]); | ||
|
||
return ( | ||
<div> | ||
<div | ||
css={{ | ||
display: "flex", | ||
gap: 8, | ||
}} | ||
> | ||
<TextField | ||
{...textFieldProps} | ||
fullWidth | ||
value={state.durationFieldValue} | ||
onChange={(e) => { | ||
const durationFieldValue = intMask(e.currentTarget.value); | ||
|
||
dispatch({ | ||
type: "CHANGE_DURATION_FIELD_VALUE", | ||
fieldValue: durationFieldValue, | ||
}); | ||
|
||
const newDurationInMs = durationInMs( | ||
durationFieldValue, | ||
state.unit, | ||
); | ||
if (newDurationInMs !== parentValueMs) { | ||
onChange(newDurationInMs); | ||
} | ||
}} | ||
inputProps={{ | ||
step: 1, | ||
}} | ||
BrunoQuaresma marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/> | ||
<Select | ||
disabled={props.disabled} | ||
css={{ width: 120, "& .MuiSelect-icon": { padding: 2 } }} | ||
value={state.unit} | ||
onChange={(e) => { | ||
const unit = e.target.value as TimeUnit; | ||
dispatch({ | ||
type: "CHANGE_TIME_UNIT", | ||
unit, | ||
}); | ||
}} | ||
inputProps={{ "aria-label": "Time unit" }} | ||
IconComponent={KeyboardArrowDown} | ||
> | ||
<MenuItem value="hours">Hours</MenuItem> | ||
<MenuItem | ||
value="days" | ||
disabled={!canConvertDurationToDays(currentDurationMs)} | ||
> | ||
Days | ||
</MenuItem> | ||
</Select> | ||
</div> | ||
|
||
{helperText && ( | ||
<FormHelperText error={props.error}>{helperText}</FormHelperText> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was there a reason why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because it is used by the TextField props as well. |
||
)} | ||
</div> | ||
); | ||
}; | ||
|
||
function initState(value: number): State { | ||
const unit = suggestedTimeUnit(value); | ||
const durationFieldValue = | ||
unit === "hours" | ||
? durationInHours(value).toString() | ||
: durationInDays(value).toString(); | ||
|
||
return { | ||
unit, | ||
durationFieldValue, | ||
}; | ||
} | ||
|
||
function intMask(value: string): string { | ||
return value.replace(/\D/g, ""); | ||
} | ||
|
||
function durationInMs(durationFieldValue: string, unit: TimeUnit): number { | ||
const durationInMs = parseInt(durationFieldValue, 10); | ||
|
||
if (Number.isNaN(durationInMs)) { | ||
return 0; | ||
} | ||
|
||
return unit === "hours" | ||
? hoursToDuration(durationInMs) | ||
: daysToDuration(durationInMs); | ||
} | ||
|
||
function hoursToDuration(hours: number): number { | ||
return hours * 60 * 60 * 1000; | ||
} | ||
|
||
function daysToDuration(days: number): number { | ||
return days * 24 * hoursToDuration(1); | ||
} | ||
|
||
function canConvertDurationToDays(duration: number): boolean { | ||
return Number.isInteger(durationInDays(duration)); | ||
} |
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.
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.
Just making sure: this is just a precaution/double-bookkeeping, right? Even though the UI has the
disabled
check to prevent the days unit from being selected at certain points, the reducer is also enforcing that the state update can't go through, in case the UI is set up wrong?Uh oh!
There was an error while loading. Please reload this page.
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.
Yes, I think it can be useful to have it in the reducer + disabled attribute. Wdyt?