Skip to content

Commit 998c0cf

Browse files
mirekmcursoragent
mirekm
and
cursoragent
authored
Fix attribute unassign null and bulk unassign (#6828)
* Fix attribute unassign sending a null id * Keep assigned-attribute columns stable with a name link and trash icon. A row <a display:contents> centered names, and a labeled Unassign button widened the actions column. Link the name instead, reserve the drag column in the header, and use the same trash icon as the row so selection does not shift checkboxes or actions. Co-authored-by: Cursor <cursoragent@cursor.com> * Stop assigned-attribute checkboxes from opening the attribute page Inside the type-page form, Radix stops the checkbox click so the browser follows the name link. Cancel that default on the control and keep the name hit target off the checkbox. * Add changesets * Fix strict typing on assigned-attribute row selection * Prefer the URL attribute id while the unassign dialog is open A cancelled click left a stale captured id that could win over a later ?id=. Fall back to the click-time id only after the URL id is cleared. --------- Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 59e8dbe commit 998c0cf

25 files changed

Lines changed: 915 additions & 185 deletions

‎.changeset/type-attribute-list-selection.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"saleor-dashboard": patch
3+
---
4+
5+
Fix the assigned-attribute lists on product types and model types: the row checkbox no longer opens the attribute, selecting rows no longer shifts the columns, and bulk unassign is the same trash icon as the row action.

‎.changeset/type-attribute-unassign-id.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"saleor-dashboard": patch
3+
---
4+
5+
Fix unassigning an attribute from a product type or model type. The confirm dialog now keeps the attribute id, so the request no longer sends `null` and the attribute is actually removed.

‎src/attributes/components/AssignedAttributesCard/AssignedAttributesBulkDeleteButton.tsx‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import { iconSize, iconStrokeWidthBySize } from "@dashboard/components/icons";
2+
import { Button } from "@saleor/macaw-ui-next";
3+
import { Trash2 } from "lucide-react";
4+
5+
interface AssignedAttributesBulkDeleteButtonProps {
6+
onClick: () => void;
7+
label: string;
8+
}
9+
10+
/** Icon-only bulk unassign — same size as the row trash so the actions column does not shift. */
11+
export const AssignedAttributesBulkDeleteButton = ({
12+
onClick,
13+
label,
14+
}: AssignedAttributesBulkDeleteButtonProps): JSX.Element => (
15+
<Button
16+
data-test-id="bulk-delete-button"
17+
variant="tertiary"
18+
type="button"
19+
onClick={onClick}
20+
title={label}
21+
aria-label={label}
22+
icon={<Trash2 size={iconSize.small} strokeWidth={iconStrokeWidthBySize.small} />}
23+
/>
24+
);

‎src/attributes/components/AssignedAttributesCard/AssignedAttributesCard.module.css‎

Lines changed: 0 additions & 3 deletions
This file was deleted.

‎src/attributes/components/AssignedAttributesCard/AssignedAttributesCard.test.tsx‎

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
import { attributeUrl } from "@dashboard/attributes/urls";
2+
import { AttributeInputTypeEnum } from "@dashboard/graphql";
3+
import Wrapper from "@test/wrapper";
4+
import { render, screen, within } from "@testing-library/react";
5+
import userEvent from "@testing-library/user-event";
6+
import { createMemoryHistory, type MemoryHistory } from "history";
7+
import { Router } from "react-router-dom";
8+
9+
import { AssignedAttributesBulkDeleteButton } from "./AssignedAttributesBulkDeleteButton";
10+
import { type AssignedAttributeListItem, AssignedAttributesCard } from "./AssignedAttributesCard";
11+
12+
// Radix mounts a hidden input inside <form>; jsdom has no ResizeObserver.
13+
global.ResizeObserver = jest.fn().mockImplementation(() => ({
14+
observe: jest.fn(),
15+
unobserve: jest.fn(),
16+
disconnect: jest.fn(),
17+
}));
18+
19+
const attributeId = "QXR0cmlidXRlOjE=";
20+
const listPath = "/model-types/1";
21+
22+
const attributes: AssignedAttributeListItem[] = [
23+
{
24+
id: attributeId,
25+
name: "Author",
26+
slug: "author",
27+
inputType: AttributeInputTypeEnum.DROPDOWN,
28+
valueRequired: true,
29+
},
30+
];
31+
32+
const renderCard = (
33+
toggle: jest.Mock = jest.fn(),
34+
): { history: MemoryHistory; toggle: jest.Mock } => {
35+
const history = createMemoryHistory({ initialEntries: [listPath] });
36+
37+
render(
38+
<Router history={history}>
39+
<form>
40+
<AssignedAttributesCard
41+
attributes={attributes}
42+
disabled={false}
43+
title="Attributes"
44+
intro="Assigned attributes"
45+
empty="No attributes"
46+
cardTestId="page-attributes"
47+
assignTestId="assign-attributes"
48+
createTestId="create-attribute"
49+
createOptionLabel="Create attribute"
50+
skeletonTestId="page-attributes-skeleton"
51+
isChecked={() => false}
52+
selected={0}
53+
toggle={toggle}
54+
toggleAll={jest.fn()}
55+
toolbar={null}
56+
onAttributeAssign={jest.fn()}
57+
onAttributeCreate={jest.fn()}
58+
onAttributeReorder={jest.fn()}
59+
onAttributeUnassign={jest.fn()}
60+
/>
61+
</form>
62+
</Router>,
63+
{ wrapper: Wrapper },
64+
);
65+
66+
return { history, toggle };
67+
};
68+
69+
describe("AssignedAttributesCard selection header", () => {
70+
it("keeps value-required and unassign in their columns when rows are selected", () => {
71+
// Arrange
72+
const history = createMemoryHistory({ initialEntries: [listPath] });
73+
74+
render(
75+
<Router history={history}>
76+
<AssignedAttributesCard
77+
attributes={attributes}
78+
disabled={false}
79+
title="Attributes"
80+
intro="Assigned attributes"
81+
empty="No attributes"
82+
cardTestId="page-attributes"
83+
assignTestId="assign-attributes"
84+
createTestId="create-attribute"
85+
createOptionLabel="Create attribute"
86+
skeletonTestId="page-attributes-skeleton"
87+
isChecked={() => true}
88+
selected={1}
89+
toggle={jest.fn()}
90+
toggleAll={jest.fn()}
91+
toolbar={<AssignedAttributesBulkDeleteButton onClick={jest.fn()} label="Unassign" />}
92+
onAttributeAssign={jest.fn()}
93+
onAttributeCreate={jest.fn()}
94+
onAttributeReorder={jest.fn()}
95+
onAttributeUnassign={jest.fn()}
96+
/>
97+
</Router>,
98+
{ wrapper: Wrapper },
99+
);
100+
101+
// Assert
102+
const headerRow = screen.getByTestId("SelectedText").closest("tr");
103+
const bodyRow = screen.getByText("Author").closest("tr");
104+
105+
if (!headerRow || !bodyRow) {
106+
throw new Error("Expected header and body rows");
107+
}
108+
109+
// Assert
110+
expect(screen.getByTestId("SelectedText")).toHaveTextContent("Selected 1 item");
111+
expect(screen.getByText("Value required")).toBeInTheDocument();
112+
expect(screen.getByRole("button", { name: "Unassign" })).toBeInTheDocument();
113+
expect(screen.getByTestId("bulk-delete-button")).toBeInTheDocument();
114+
expect(headerRow.querySelectorAll("th")).toHaveLength(bodyRow.querySelectorAll("td").length);
115+
expect(headerRow.querySelectorAll("th")[0]).toHaveAttribute(
116+
"data-test-id",
117+
"drag-column-spacer",
118+
);
119+
expect(
120+
within(headerRow.querySelectorAll("th")[1] as HTMLElement).getByTestId("select-all-checkbox"),
121+
).toBeInTheDocument();
122+
});
123+
});
124+
125+
describe("AssignedAttributesCard name link", () => {
126+
it("navigates to the attribute from the name, not the row", async () => {
127+
// Arrange
128+
const user = userEvent.setup();
129+
const { history } = renderCard();
130+
131+
// Act
132+
await user.click(screen.getByText("Author"));
133+
134+
// Assert
135+
expect(history.location.pathname).toBe(attributeUrl(attributeId).split("?")[0]);
136+
});
137+
});
138+
139+
describe("AssignedAttributesCard row checkbox", () => {
140+
it("toggles without navigating to the attribute", async () => {
141+
// Arrange
142+
const user = userEvent.setup();
143+
const { history, toggle } = renderCard();
144+
const row = screen.getByText("Author").closest("tr");
145+
146+
if (!row) {
147+
throw new Error("Expected a table row for the Author attribute");
148+
}
149+
150+
// Act
151+
await user.click(within(row).getByRole("checkbox"));
152+
153+
// Assert
154+
expect(toggle).toHaveBeenCalledWith(attributeId);
155+
expect(history.location.pathname).toBe(listPath);
156+
});
157+
});

‎src/attributes/components/AssignedAttributesCard/AssignedAttributesCard.tsx‎

Lines changed: 48 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -9,25 +9,24 @@ import { ASSIGNABLE_LIST_TABLE_ACTION_INSET } from "@dashboard/components/Assign
99
import { AttributeNameWithTypeIcon } from "@dashboard/components/AttributeInputTypeIcon/AttributeNameWithTypeIcon";
1010
import { ButtonGroupWithDropdown } from "@dashboard/components/ButtonGroupWithDropdown";
1111
import { iconSize, iconStrokeWidthBySize } from "@dashboard/components/icons";
12+
import { Link } from "@dashboard/components/Link";
1213
import { Placeholder } from "@dashboard/components/Placeholder";
1314
import { ResponsiveTable, tableStyles } from "@dashboard/components/ResponsiveTable";
1415
import { SortableTableBody, SortableTableRow } from "@dashboard/components/SortableTable";
1516
import { TableButtonWrapper } from "@dashboard/components/TableButtonWrapper/TableButtonWrapper";
1617
import TableHead from "@dashboard/components/TableHead";
18+
import { TableRowLinkCheckbox } from "@dashboard/components/TableRowLink/TableRowLinkCheckbox";
1719
import { type AttributeInputTypeEnum } from "@dashboard/graphql";
1820
import { useOptimisticListReorder } from "@dashboard/hooks/useOptimisticListReorder";
1921
import { buttonMessages } from "@dashboard/intl";
2022
import { Ripple } from "@dashboard/ripples/components/Ripple";
2123
import { type ListActions, type ReorderAction } from "@dashboard/types";
2224
import { TableBody, TableCell } from "@material-ui/core";
23-
import { Box, Button, Checkbox, Skeleton, Text } from "@saleor/macaw-ui-next";
24-
import clsx from "clsx";
25+
import { Box, Button, Skeleton, Text } from "@saleor/macaw-ui-next";
2526
import { Trash2 } from "lucide-react";
26-
import { type MouseEvent, type ReactNode } from "react";
27+
import { type ReactNode } from "react";
2728
import { FormattedMessage, useIntl } from "react-intl";
2829

29-
import styles from "./AssignedAttributesCard.module.css";
30-
3130
export interface AssignedAttributeListItem {
3231
id: string;
3332
name: string | null;
@@ -54,11 +53,6 @@ interface AssignedAttributesCardProps extends ListActions {
5453
onAttributeUnassign: (id: string) => void;
5554
}
5655

57-
const stopRowNavigation = (event: MouseEvent): void => {
58-
event.preventDefault();
59-
event.stopPropagation();
60-
};
61-
6256
export const AssignedAttributesCard = ({
6357
attributes,
6458
disabled,
@@ -141,15 +135,25 @@ export const AssignedAttributesCard = ({
141135
compact
142136
disabled={disabled || isLoading}
143137
dragRows
138+
keepColumnHeaders
144139
selected={selected}
145140
items={isLoading ? undefined : orderedAttributes}
146141
toggleAll={toggleAll}
147-
toolbar={toolbar}
148142
>
149143
<TableCell>
150-
<Text size={2} lineHeight={2} color="default2">
151-
<FormattedMessage id="kTr2o8" defaultMessage="Attribute name" />
152-
</Text>
144+
{selected > 0 ? (
145+
<Text data-test-id="SelectedText" size={2} lineHeight={2}>
146+
<FormattedMessage
147+
id="imYtnq"
148+
defaultMessage="Selected {number, plural, one {# item} other {# items}}"
149+
values={{ number: selected }}
150+
/>
151+
</Text>
152+
) : (
153+
<Text size={2} lineHeight={2} color="default2">
154+
<FormattedMessage id="kTr2o8" defaultMessage="Attribute name" />
155+
</Text>
156+
)}
153157
</TableCell>
154158
<TableCell className={columnStyles.colValueRequired}>
155159
<Text size={2} lineHeight={2} color="default2">
@@ -159,7 +163,20 @@ export const AssignedAttributesCard = ({
159163
{showVariantSpacer ? (
160164
<TableCell className={columnStyles.colVariant} aria-hidden />
161165
) : null}
162-
<TableCell />
166+
<TableCell className={tableStyles.actionsCell}>
167+
{selected > 0 && toolbar ? (
168+
<Box
169+
display="flex"
170+
alignItems="center"
171+
justifyContent="flex-end"
172+
width="100%"
173+
height="100%"
174+
paddingRight={ASSIGNABLE_LIST_TABLE_ACTION_INSET}
175+
>
176+
{toolbar}
177+
</Box>
178+
) : null}
179+
</TableCell>
163180
</TableHead>
164181
{isLoading ? (
165182
<TableBody data-test-id={skeletonTestId} aria-busy="true">
@@ -168,41 +185,36 @@ export const AssignedAttributesCard = ({
168185
) : (
169186
<SortableTableBody onSortEnd={onSortEnd}>
170187
{orderedAttributes.map((attribute, attributeIndex) => {
171-
const isSelected = isChecked(attribute.id);
188+
const isSelected = isChecked(attribute.id) === true;
172189

173190
return (
174191
<SortableTableRow
175192
selected={isSelected}
176-
className={clsx(styles.link, tableStyles.row)}
193+
className={tableStyles.row}
177194
hover
178-
href={attributeUrl(attribute.id)}
179195
key={attribute.id}
180196
id={attribute.id}
181197
index={attributeIndex || 0}
182198
data-test-id={"id-" + attribute.id}
183199
>
184200
<TableCell className={tableStyles.checkboxCell}>
185-
<Box
186-
display="flex"
187-
alignItems="center"
188-
height="100%"
189-
onClick={stopRowNavigation}
190-
onMouseDown={stopRowNavigation}
191-
>
192-
<Checkbox
193-
checked={isSelected}
194-
disabled={disabled}
195-
onCheckedChange={() => toggle(attribute.id)}
196-
/>
197-
</Box>
201+
<TableRowLinkCheckbox
202+
checked={isSelected}
203+
disabled={disabled}
204+
onCheckedChange={() => toggle(attribute.id)}
205+
/>
198206
</TableCell>
199207
<TableCell data-test-id="name">
200208
{attribute.name ? (
201-
<AttributeNameWithTypeIcon
202-
name={attribute.name}
203-
inputType={attribute.inputType}
204-
secondary={attribute.slug}
205-
/>
209+
<Box display="inline-flex" maxWidth="100%">
210+
<Link href={attributeUrl(attribute.id)} color="secondary">
211+
<AttributeNameWithTypeIcon
212+
name={attribute.name}
213+
inputType={attribute.inputType}
214+
secondary={attribute.slug}
215+
/>
216+
</Link>
217+
</Box>
206218
) : (
207219
<Skeleton />
208220
)}

‎src/attributes/components/AttributeValues/AttributeValues.tsx‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { SearchInput } from "@dashboard/components/SearchInput/SearchInput";
1212
import { SortableTableBody, SortableTableRow } from "@dashboard/components/SortableTable";
1313
import { TableButtonWrapper } from "@dashboard/components/TableButtonWrapper/TableButtonWrapper";
1414
import TableHead from "@dashboard/components/TableHead";
15+
import { stopTableRowLinkNavigation } from "@dashboard/components/TableRowLink/stopTableRowLinkNavigation";
1516
import { PAGINATE_BY } from "@dashboard/config";
1617
import {
1718
type AttributeErrorFragment,
@@ -119,8 +120,8 @@ const getColumnClassName = (
119120
return clsx(baseClassMap[column], isEmbedded && embeddedClassMap[column]);
120121
};
121122

122-
const stopRowClick = (event: MouseEvent) => {
123-
event.stopPropagation();
123+
const stopRowClick = (event: MouseEvent): void => {
124+
stopTableRowLinkNavigation(event);
124125
};
125126

126127
const AttributeValues = ({

0 commit comments

Comments
 (0)