Skip to content

Commit

Permalink
fix: Fix select all with disabled rows.
Browse files Browse the repository at this point in the history
  • Loading branch information
stephenh committed Oct 19, 2023
1 parent 06648fb commit 4c23736
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 17 deletions.
58 changes: 53 additions & 5 deletions src/components/Table/GridTable.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,6 @@ const nestedColumns: GridColumn<NestedRow>[] = [
},
selectColumn<NestedRow>({
totals: emptyCell,
header: (data, { row }) => ({ content: <Select id={row.id} /> }),
parent: (data, { row }) => ({ content: <Select id={row.id} /> }),
child: (data, { row }) => ({ content: <Select id={row.id} /> }),
grandChild: (data, { row }) => ({ content: <Select id={row.id} /> }),
}),
{
totals: emptyCell,
Expand Down Expand Up @@ -1647,6 +1643,57 @@ describe("GridTable", () => {
expect(api.current!.getSelectedRowIds()).toEqual([]);
});

it("can select all with disabled children", async () => {
// Given a parent with two children
const rows: GridDataRow<NestedRow>[] = [
simpleHeader,
{
...{ kind: "parent", id: "p1", data: { name: "parent 1" } },
children: [
{ kind: "child", id: "p1c1", data: { name: "child p1c1" } },
// And the 2nd one is disabled
{ kind: "child", id: "p1c2", data: { name: "child p1c2" }, selectable: false },
],
},
];

const api: MutableRefObject<GridTableApi<NestedRow> | undefined> = { current: undefined };
function Test() {
const _api = useGridTableApi<NestedRow>();
api.current = _api;
return <GridTable<NestedRow> api={_api} columns={nestedColumns} rows={rows} />;
}
const r = await render(<Test />);

// And all three rows are initially rendered
expect(cell(r, 1, 2)).toHaveTextContent("parent 1");
expect(cell(r, 2, 2)).toHaveTextContent("child p1c1");
expect(cell(r, 3, 2)).toHaveTextContent("child p1c2");

// When we select all
click(cell(r, 0, 1).children[0] as any);
// Then the 'All', Parent, and 1st child rows are shown as selected
expect(cellAnd(r, 0, 1, "select")).toBeChecked();
expect(cellAnd(r, 1, 1, "select")).toBeChecked();
expect(cellAnd(r, 2, 1, "select")).toBeChecked();
// But the 2nd child is not
expect(cellAnd(r, 3, 1, "select")).toBeDisabled();
// And the api can fetch them
expect(api.current!.getSelectedRowIds()).toEqual(["p1", "p1c1"]);
expect(api.current!.getSelectedRowIds("child")).toEqual(["p1c1"]);

// And when we unselect all
click(cell(r, 0, 1).children[0] as any);
// Then all rows are shown as unselected
expect(cellAnd(r, 0, 1, "select")).not.toBeChecked();
expect(cellAnd(r, 1, 1, "select")).not.toBeChecked();
expect(cellAnd(r, 2, 1, "select")).not.toBeChecked();
expect(cellAnd(r, 3, 1, "select")).not.toBeChecked();
expect(cellAnd(r, 3, 1, "select")).toBeDisabled();
// And they are no longer selected
expect(api.current!.getSelectedRowIds()).toEqual([]);
});

it("fires props.onSelect", async () => {
// Given a parent with a child
const actions: string[] = [];
Expand Down Expand Up @@ -3566,7 +3613,8 @@ describe("GridTable", () => {
// When deselecting the visible row
click(r.select_2);

// Then the parent row is now unchecked (respecting only the matched rows), but the header is still partially selected as it takes into consideration the kept selected rows
// Then the parent row is now unchecked (respecting only the matched rows), but the header is still partially
// selected as it takes into consideration the kept selected rows
expect(cellAnd(r, 0, 1, "select")).toHaveAttribute("data-indeterminate", "true");
expect(cellAnd(r, 2, 1, "select")).not.toBeChecked();

Expand Down
4 changes: 1 addition & 3 deletions src/components/Table/components/SelectToggle.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@ export function SelectToggle({ id, disabled }: SelectToggleProps) {
checkboxOnly={true}
disabled={disabled}
label="Select"
onChange={(selected) => {
tableState.selectRow(id, selected);
}}
onChange={(selected) => tableState.selectRow(id, selected)}
selected={selected}
/>
);
Expand Down
13 changes: 5 additions & 8 deletions src/components/Table/utils/RowState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,7 @@ export class RowState<R extends Kinded> {
removed: false | "soft" | "hard" = false;
private isCalculatingDirectMatch = false;

constructor(
private states: RowStates<R>,
public parent: RowState<R> | undefined,
row: GridDataRow<R>,
) {
constructor(private states: RowStates<R>, public parent: RowState<R> | undefined, row: GridDataRow<R>) {
this.row = row;
this.selected = !!row.initSelected;
this.collapsed = states.storage.wasCollapsed(row.id) ?? !!row.initCollapsed;
Expand Down Expand Up @@ -127,7 +123,7 @@ export class RowState<R extends Kinded> {
// client-side filter changes, a child reappears, and we need to transition to partial-ness.
if (this.isParent) {
// Use visibleChildren b/c if filters are hiding some of our children, we still want to show fully selected
const children = this.visibleChildren;
const children = this.visibleChildren.filter((c) => c.row.selectable !== false);
const allChecked = children.every((child) => child.selectedState === "checked");
const allUnchecked = children.every((child) => child.selectedState === "unchecked");
return children.length === 0 ? "unchecked" : allChecked ? "checked" : allUnchecked ? "unchecked" : "partial";
Expand All @@ -143,7 +139,7 @@ export class RowState<R extends Kinded> {
*/
get selectedStateForHeader(): SelectedState {
if (this.children) {
const children = this.visibleChildren;
const children = this.visibleChildren.filter((c) => c.row.selectable !== false || c.isParent);
const allChecked = children.every((child) => child.selectedStateForHeader === "checked");
const allUnchecked = children.every((child) => child.selectedStateForHeader === "unchecked");
// For the header purposes, if this is a selectable-row (i.e. not inferSelectedState) make sure
Expand Down Expand Up @@ -302,7 +298,8 @@ export class RowState<R extends Kinded> {
* they want the row to be selectable.
*/
private get isParent(): boolean {
return !!this.children && this.children.length > 0 && this.inferSelectedState;
// Check for KEPT_GROUP b/c it has `this.children = []` but we synthesize its children in `visibleChildren`
return !!this.children && (this.children.length > 0 || this.row.id === KEPT_GROUP) && this.inferSelectedState;
}

private get isPinned(): boolean {
Expand Down
5 changes: 5 additions & 0 deletions src/inputs/Checkbox.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ describe("Checkbox", () => {
expect(r.customId).toBeTruthy();
});

it("sets disabled", async () => {
const r = await render(<Checkbox label="Test" selected={false} onChange={noop} disabled={true} />);
expect(r.test).toBeDisabled();
});

it("treats indeterminate as false", async () => {
// Given an indeterminate checkbox
const onChange = jest.fn();
Expand Down
2 changes: 1 addition & 1 deletion src/inputs/CheckboxBase.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export function CheckboxBase(props: CheckboxBaseProps) {
<VisuallyHidden>
<input ref={ref} {...mergeProps(inputProps, focusProps)} {...tid} data-indeterminate={isIndeterminate} />
</VisuallyHidden>
<StyledCheckbox {...props} isFocusVisible={isFocusVisible} />
<StyledCheckbox {...props} isFocusVisible={isFocusVisible} {...tid} />
{!checkboxOnly && (
// Use a mtPx(-2) to better align the label with the checkbox.
// Not using align-items: center as the checkbox would align with all content below, where we really want it to stay only aligned with the label
Expand Down

0 comments on commit 4c23736

Please sign in to comment.