Skip to content

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 18 commits into from
May 17, 2024
Merged
Show file tree
Hide file tree
Changes from 15 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
44 changes: 44 additions & 0 deletions site/src/components/DurationField/DurationField.stories.tsx
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;
}
187 changes: 187 additions & 0 deletions site/src/components/DurationField/DurationField.tsx
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;
}
Comment on lines +48 to +53
Copy link
Member

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?

Copy link
Collaborator Author

@BrunoQuaresma BrunoQuaresma May 17, 2024

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?


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,
}}
/>
<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>
Copy link
Member

Choose a reason for hiding this comment

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

Was there a reason why props.error wasn't destructured from the props at the top of the component?

Copy link
Collaborator Author

@BrunoQuaresma BrunoQuaresma May 17, 2024

Choose a reason for hiding this comment

The 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));
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { humanDuration } from "utils/time";

const hours = (h: number) => (h === 1 ? "hour" : "hours");
const days = (d: number) => (d === 1 ? "day" : "days");

export const DefaultTTLHelperText = (props: { ttl?: number }) => {
const { ttl = 0 } = props;
Expand Down Expand Up @@ -60,7 +61,7 @@ export const FailureTTLHelperText = (props: { ttl?: number }) => {

return (
<span>
Coder will attempt to stop failed workspaces after {ttl} {days(ttl)}.
Coder will attempt to stop failed workspaces after {humanDuration(ttl)}.
</span>
);
};
Expand All @@ -79,8 +80,8 @@ export const DormancyTTLHelperText = (props: { ttl?: number }) => {

return (
<span>
Coder will mark workspaces as dormant after {ttl} {days(ttl)} without user
connections.
Coder will mark workspaces as dormant after {humanDuration(ttl)} without
user connections.
</span>
);
};
Expand All @@ -99,8 +100,8 @@ export const DormancyAutoDeletionTTLHelperText = (props: { ttl?: number }) => {

return (
<span>
Coder will automatically delete dormant workspaces after {ttl} {days(ttl)}
.
Coder will automatically delete dormant workspaces after{" "}
{humanDuration(ttl)}.
</span>
);
};
Loading
Loading