Skip to content

feat(site): support icon and description in preset #19063

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.

Sign up for GitHub

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 12 commits into from
Jul 29, 2025
Merged

feat(site): support icon and description in preset #19063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8fea22d
feat: UI create workspace page with preset's icon and description
ssncferreira Jul 28, 2025
e1e3d37
fix(site): update Combobox to use ExternalImage instead of Avatar
ssncferreira Jul 28, 2025
9b5e5b7
fix: CreateWorkspacePageView stories
ssncferreira Jul 29, 2025
8a1aa2c
Merge remote-tracking branch 'origin/main' into ssncferreira/feat-ui-…
ssncferreira Jul 29, 2025
477ce03
fix: combobox stories tests
ssncferreira Jul 29, 2025
3256320
chore: minor fixes suggested by Copilot
ssncferreira Jul 29, 2025
b3e8854
fix: fix width to w-72 in container instead of Combobox
jaaydenh Jul 29, 2025
f95e375
fix: adjust Combobox' Button and PopoverContent width
ssncferreira Jul 29, 2025
d895d42
fix: close combobox dropdown when option is selected
ssncferreira Jul 29, 2025
da8bab8
Merge branch 'main' into ssncferreira/feat-ui-preset-icon-description
jaaydenh Jul 29, 2025
c4444ec
fix: None option with 'undefined' value for correct highlight
ssncferreira Jul 29, 2025
a46a698
Merge remote-tracking branch 'origin/main' into ssncferreira/feat-ui-…
ssncferreira Jul 29, 2025
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
9 changes: 6 additions & 3 deletions site/src/components/Combobox/Combobox.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ export const SearchAndFilter: Story = {
screen.queryByRole("option", { name: "Kotlin" }),
).not.toBeInTheDocument();
});
await userEvent.click(screen.getByRole("option", { name: "Rust" }));
// Accessible name includes both image alt text and text content: "Rust Rust"
await userEvent.click(screen.getByRole("option", { name: "Rust Rust" }));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we changed the icon image to be an ExternalImage with alt={option.displayName} https://github.com/coder/coder/pull/19063/files#diff-f7c0d4d4c43b6dd17458ff44a1a0dfdccfae367b09ffe41fdbe09c48c151e466R126
The name includes both the image alt text (Rust) + the text content (Rust). Let me know if this is ok, or if there is a better way to filter out this element.

Copy link
Member

@Parkreiner Parkreiner Jul 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry to come in at the last minute (after things have been merged), but these test changes are papering over accessibility problems that the new code introduced. This is especially relevant, as there's a good chance that I'm going to be submitting a VPAT (Voluntary Product Accessibility Template) audit for coder/coder sometime in H2 for Marketing/GTM

There's a reason why the testing library changed the accessible name to "Rust Rust" – because that's the value that a screen reader will read out to a user who can't see. It's going to be super annoying for the user if they get doubled-up announcements as they cycle through every item in the options list

In my mind, what we can do instead is:

  1. Revert the accessible names for the test selectors to remove the duplicated values (so just Rust and just Go, rather than Rust Rust and Go Go)
  2. Set the alt text to an empty string

Alt text is only valuable if the content of the image isn't strictly decorative, and actually has semantic meaning that other content in the UI isn't capturing. Because we have the name right next to the image, we don't need the alt text. And or UI programming specifically, you can get away with empty alt text more often than you would think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Parkreiner Thank you for letting me know about this, this is really helpful context 👍

You're right: this PR introduced alt={option.displayName}, which led to the repeated accessible name (e.g. "Rust Rust"). I hadn’t realized that would have a direct impact on screen readers, that's super useful to know.

I’ll go ahead and:

  • Revert the alt attribute to use an empty string.
  • Update the tests to reflect the correct accessible names without duplication.

Appreciate you taking the time to call this out 🙂

},
};

Expand Down Expand Up @@ -137,9 +138,11 @@ export const ClearSelectedOption: Story = {
await userEvent.click(canvas.getByRole("button"));
// const goOption = screen.getByText("Go");
// First select an option
await userEvent.click(await screen.findByRole("option", { name: "Go" }));
// Accessible name includes both image alt text and text content: "Go Go"
await userEvent.click(await screen.findByRole("option", { name: "Go Go" }));
// Then clear it by selecting it again
await userEvent.click(await screen.findByRole("option", { name: "Go" }));
// Accessible name includes both image alt text and text content: "Go Go"
await userEvent.click(await screen.findByRole("option", { name: "Go Go" }));

await waitFor(() =>
expect(canvas.getByRole("button")).toHaveTextContent("Select option"),
Expand Down
39 changes: 22 additions & 17 deletions site/src/components/Combobox/Combobox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { Avatar } from "components/Avatar/Avatar";
import { Button } from "components/Button/Button";
import {
Command,
Expand All @@ -23,6 +22,7 @@ import { Check, ChevronDown, CornerDownLeft } from "lucide-react";
import { Info } from "lucide-react";
import { type FC, type KeyboardEventHandler, useState } from "react";
import { cn } from "utils/cn";
import { ExternalImage } from "../ExternalImage/ExternalImage";

interface ComboboxProps {
value: string;
Expand Down Expand Up @@ -69,27 +69,26 @@ export const Combobox: FC<ComboboxProps> = ({

const isOpen = open ?? managedOpen;

const handleOpenChange = (newOpen: boolean) => {
setManagedOpen(newOpen);
onOpenChange?.(newOpen);
};

return (
<Popover
open={isOpen}
onOpenChange={(newOpen) => {
setManagedOpen(newOpen);
onOpenChange?.(newOpen);
}}
>
<Popover open={isOpen} onOpenChange={handleOpenChange}>
<PopoverTrigger asChild>
<Button
variant="outline"
aria-expanded={isOpen}
className="w-72 justify-between group"
className="w-full justify-between group"
>
<span className={cn(!value && "text-content-secondary")}>
{optionsMap.get(value)?.displayName || value || placeholder}
</span>
<ChevronDown className="size-icon-sm text-content-secondary group-hover:text-content-primary" />
</Button>
</PopoverTrigger>
<PopoverContent className="w-72">
<PopoverContent className="w-[var(--radix-popover-trigger-width)]">
<Command>
<CommandInput
placeholder="Search or enter custom value"
Expand All @@ -116,15 +115,21 @@ export const Combobox: FC<ComboboxProps> = ({
keywords={[option.displayName]}
onSelect={(currentValue) => {
onSelect(currentValue === value ? "" : currentValue);
// Close the popover after selection
handleOpenChange(false);
}}
>
{showIcons && (
<Avatar
size="sm"
src={option.icon}
fallback={option.value}
/>
)}
{showIcons &&
(option.icon ? (
<ExternalImage
className="w-4 h-4 object-contain"
src={option.icon}
alt={option.displayName}
/>
) : (
/* Placeholder for missing icon to maintain layout consistency */
<div className="w-4 h-4"></div>
))}
{option.displayName}
<div className="flex flex-row items-center ml-auto gap-1">
{value === option.value && (
Expand Down
59 changes: 24 additions & 35 deletions site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { action } from "@storybook/addon-actions";
import type { Meta, StoryObj } from "@storybook/react";
import { expect, screen, waitFor } from "@storybook/test";
import { within } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import { chromatic } from "testHelpers/chromatic";
Expand Down Expand Up @@ -129,8 +130,8 @@ export const PresetsButNoneSelected: Story = {
{
ID: "preset-1",
Name: "Preset 1",
Description: "",
Icon: "",
Description: "Preset 1 description",
Icon: "/emojis/0031-fe0f-20e3.png",
Default: false,
Parameters: [
{
Expand All @@ -143,9 +144,8 @@ export const PresetsButNoneSelected: Story = {
{
ID: "preset-2",
Name: "Preset 2",
Description:
"Lorem ipsum dolor sit amet, consectetur adipiscing elit. Suspendisse imperdiet ultricies massa, eu dapibus ex fermentum ac.",
Icon: "/emojis/1f60e.png",
Description: "Preset 2 description",
Icon: "/emojis/0032-fe0f-20e3.png",
Default: false,
Parameters: [
{
Expand All @@ -165,21 +165,12 @@ export const PresetsButNoneSelected: Story = {
};

export const PresetSelected: Story = {
args: PresetsButNoneSelected.args,
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
await userEvent.click(canvas.getByLabelText("Preset"));
await userEvent.click(canvas.getByText("Preset 1"));
},
};

export const PresetSelectedWithHiddenParameters: Story = {
args: PresetsButNoneSelected.args,
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
// Select a preset
await userEvent.click(canvas.getByLabelText("Preset"));
await userEvent.click(canvas.getByText("Preset 1"));
await userEvent.click(canvas.getByRole("button", { name: "None" }));
await userEvent.click(screen.getByText("Preset 1"));
},
};

Expand All @@ -188,8 +179,8 @@ export const PresetSelectedWithVisibleParameters: Story = {
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
// Select a preset
await userEvent.click(canvas.getByLabelText("Preset"));
await userEvent.click(canvas.getByText("Preset 1"));
await userEvent.click(canvas.getByRole("button", { name: "None" }));
await userEvent.click(screen.getByText("Preset 1"));
// Toggle off the show preset parameters switch
await userEvent.click(canvas.getByLabelText("Show preset parameters"));
},
Expand All @@ -201,16 +192,12 @@ export const PresetReselected: Story = {
const canvas = within(canvasElement);

// First selection of Preset 1
await userEvent.click(canvas.getByLabelText("Preset"));
await userEvent.click(
canvas.getByText("Preset 1", { selector: ".MuiMenuItem-root" }),
);
await userEvent.click(canvas.getByRole("button", { name: "None" }));
await userEvent.click(screen.getByText("Preset 1"));

// Reselect the same preset
await userEvent.click(canvas.getByLabelText("Preset"));
await userEvent.click(
canvas.getByText("Preset 1", { selector: ".MuiMenuItem-root" }),
);
await userEvent.click(canvas.getByRole("button", { name: "Preset 1" }));
await userEvent.click(canvas.getByText("Preset 1"));
},
};

Expand All @@ -230,12 +217,11 @@ export const PresetNoneSelected: Story = {
const canvas = within(canvasElement);

// First select a preset to set the field value
await userEvent.click(canvas.getByLabelText("Preset"));
await userEvent.click(canvas.getByText("Preset 1"));
await userEvent.click(canvas.getByRole("button", { name: "None" }));
await userEvent.click(screen.getByText("Preset 1"));

// Then select "None" to unset the field value
await userEvent.click(canvas.getByLabelText("Preset"));
await userEvent.click(canvas.getByText("None"));
await userEvent.click(screen.getByText("None"));

// Fill in required fields and submit to test the API call
await userEvent.type(
Expand All @@ -260,8 +246,8 @@ export const PresetsWithDefault: Story = {
{
ID: "preset-1",
Name: "Preset 1",
Icon: "",
Description: "",
Description: "Preset 1 description",
Icon: "/emojis/0031-fe0f-20e3.png",
Default: false,
Parameters: [
{
Expand All @@ -274,9 +260,8 @@ export const PresetsWithDefault: Story = {
{
ID: "preset-2",
Name: "Preset 2",
Icon: "/emojis/1f60e.png",
Description:
"Lorem ipsum dolor sit amet, consectetur adipiscing elit. Suspendisse imperdiet ultricies massa, eu dapibus ex fermentum ac.",
Description: "Preset 2 description",
Icon: "/emojis/0032-fe0f-20e3.png",
Default: true,
Parameters: [
{
Expand All @@ -295,6 +280,10 @@ export const PresetsWithDefault: Story = {
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
// Should have the default preset listed first
await waitFor(() =>
expect(canvas.getByRole("button", { name: "Preset 2 (Default)" })),
);
// Wait for the switch to be available since preset parameters are populated asynchronously
await canvas.findByLabelText("Show preset parameters");
// Toggle off the show preset parameters switch
Expand Down
34 changes: 20 additions & 14 deletions site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { Alert } from "components/Alert/Alert";
import { ErrorAlert } from "components/Alert/ErrorAlert";
import { Avatar } from "components/Avatar/Avatar";
import { Button } from "components/Button/Button";
import { SelectFilter } from "components/Filter/SelectFilter";
import { Combobox } from "components/Combobox/Combobox";
import {
FormFields,
FormFooter,
Expand Down Expand Up @@ -158,16 +158,18 @@ export const CreateWorkspacePageView: FC<CreateWorkspacePageViewProps> = ({
);

const [presetOptions, setPresetOptions] = useState([
{ label: "None", value: "" },
{ displayName: "None", value: "undefined", icon: "", description: "" },
]);
const [selectedPresetIndex, setSelectedPresetIndex] = useState(0);
// Build options and keep default label/value in sync
useEffect(() => {
const options = [
{ label: "None", value: "" },
...presets.map((p) => ({
label: p.Default ? `${p.Name} (Default)` : p.Name,
value: p.ID,
{ displayName: "None", value: "undefined", icon: "", description: "" },
...presets.map((preset) => ({
displayName: preset.Default ? `${preset.Name} (Default)` : preset.Name,
value: preset.ID,
icon: preset.Icon,
description: preset.Description,
})),
];
setPresetOptions(options);
Expand Down Expand Up @@ -392,25 +394,29 @@ export const CreateWorkspacePageView: FC<CreateWorkspacePageViewProps> = ({
</Stack>
<Stack direction="column" spacing={2}>
<Stack direction="row" spacing={2}>
<SelectFilter
label="Preset"
<Combobox
value={
presetOptions[selectedPresetIndex]?.displayName || ""
}
options={presetOptions}
onSelect={(option) => {
placeholder="Select a preset"
onSelect={(value) => {
const index = presetOptions.findIndex(
(preset) => preset.value === option?.value,
(preset) => preset.value === value,
);
if (index === -1) {
return;
}
setSelectedPresetIndex(index);
form.setFieldValue(
"template_version_preset_id",
// Empty string is equivalent to using None
option?.value === "" ? undefined : option?.value,
// "undefined" string is equivalent to using None option
// Combobox requires a value in order to correctly highlight the None option
presetOptions[index].value === "undefined"
? undefined
: presetOptions[index].value,
);
}}
placeholder="Select a preset"
selectedOption={presetOptions[selectedPresetIndex]}
/>
</Stack>
{/* Only show the preset parameter visibility toggle if preset parameters are actually being modified, otherwise it has no effect. */}
Expand Down
2 changes: 1 addition & 1 deletion site/src/pages/DeploymentSettingsPage/IdpOrgSyncPage/IdpOrgSyncPageView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ export const IdpOrgSyncPageView: FC<IdpSyncPageViewProps> = ({
)}
<div className="flex flex-col gap-7">
<div className="flex flex-row pt-8 gap-2 justify-between items-start">
<div className="grid items-center gap-1">
<div className="grid items-center gap-1 w-72">
<Label className="text-sm" htmlFor={`${id}-idp-org-name`}>
IdP organization name
</Label>
Expand Down
2 changes: 1 addition & 1 deletion site/src/pages/OrganizationSettingsPage/IdpSyncPage/IdpGroupSyncForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ export const IdpGroupSyncForm: FC<IdpGroupSyncFormProps> = ({
</span>
</div>
<div className="flex flex-row gap-2 justify-between items-start">
<div className="grid items-center gap-1">
<div className="grid items-center gap-1 w-72">
<Label className="text-sm" htmlFor={`${id}-idp-group-name`}>
IdP group name
</Label>
Expand Down
2 changes: 1 addition & 1 deletion site/src/pages/OrganizationSettingsPage/IdpSyncPage/IdpRoleSyncForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ export const IdpRoleSyncForm: FC<IdpRoleSyncFormProps> = ({
<p className="text-content-danger text-sm m-0">{form.errors.field}</p>
)}
<div className="flex flex-row gap-2 justify-between items-start">
<div className="grid items-center gap-1">
<div className="grid items-center gap-1 w-72">
<Label className="text-sm" htmlFor={`${id}-idp-role-name`}>
IdP role name
</Label>
Expand Down
Loading