Skip to content

Commit

Permalink
Elevate selected filter field values (#40055) (#40070)
Browse files Browse the repository at this point in the history 
Co-authored-by: Alexander Polyankin <alexander.polyankin@metabase.com>
  • Loading branch information
@metabase-bot @ranquild
metabase-bot[bot] and ranquild committed Mar 13, 2024
1 parent 20990f2 commit 697c1eb
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 16 deletions.
126 changes: 122 additions & 4 deletions frontend/src/metabase/querying/components/FilterValuePicker/FilterValuePicker.unit.spec.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ async function setupStringPicker({
setupFieldSearchValuesEndpoints(result.field_id, value, result.values);
});

renderWithProviders(
const { rerender } = renderWithProviders(
<StringFilterValuePicker
query={query}
stageIndex={stageIndex}
Expand All @@ -72,7 +72,7 @@ async function setupStringPicker({

await waitForLoaderToBeRemoved();

return { onChange, onFocus, onBlur };
return { rerender, onChange, onFocus, onBlur };
}

async function setupNumberPicker({
Expand All @@ -91,7 +91,7 @@ async function setupNumberPicker({
setupFieldValuesEndpoints(fieldValues);
}

renderWithProviders(
const { rerender } = renderWithProviders(
<NumberFilterValuePicker
query={query}
stageIndex={stageIndex}
Expand All @@ -106,7 +106,7 @@ async function setupNumberPicker({

await waitForLoaderToBeRemoved();

return { onChange, onFocus, onBlur };
return { rerender, onChange, onFocus, onBlur };
}

describe("StringFilterValuePicker", () => {
Expand Down Expand Up @@ -242,6 +242,99 @@ describe("StringFilterValuePicker", () => {
expect(onChange).toHaveBeenCalledWith(["t", "p"]);
});

it("should elevate selected field values on initial render", async () => {
await setupStringPicker({
query,
stageIndex,
column,
values: ["p"],
fieldValues: createMockFieldValues({
field_id: PRODUCTS.CATEGORY,
values: [
["t", "To-do"],
["p", "In-progress"],
["c", "Completed"],
],
}),
});

const checkboxes = screen.getAllByRole("checkbox");
expect(checkboxes[0]).toHaveAccessibleName("In-progress");
expect(checkboxes[0]).toBeChecked();
expect(checkboxes[1]).toHaveAccessibleName("To-do");
expect(checkboxes[1]).not.toBeChecked();
expect(checkboxes[2]).toHaveAccessibleName("Completed");
expect(checkboxes[2]).not.toBeChecked();
});

it("should not elevate selected field values after checking an item", async () => {
const { rerender, onChange } = await setupStringPicker({
query,
stageIndex,
column,
values: ["p"],
fieldValues: createMockFieldValues({
field_id: PRODUCTS.CATEGORY,
values: [
["t", "To-do"],
["p", "In-progress"],
["c", "Completed"],
],
}),
});

rerender(
<StringFilterValuePicker
query={query}
stageIndex={stageIndex}
column={column}
values={["p", "c"]}
onChange={onChange}
/>,
);
const checkboxes = screen.getAllByRole("checkbox");
expect(checkboxes[0]).toHaveAccessibleName("In-progress");
expect(checkboxes[0]).toBeChecked();
expect(checkboxes[1]).toHaveAccessibleName("To-do");
expect(checkboxes[1]).not.toBeChecked();
expect(checkboxes[2]).toHaveAccessibleName("Completed");
expect(checkboxes[2]).toBeChecked();
});

it("should not elevate selected field values after unchecking an item", async () => {
const { rerender, onChange } = await setupStringPicker({
query,
stageIndex,
column,
values: ["p", "c"],
fieldValues: createMockFieldValues({
field_id: PRODUCTS.CATEGORY,
values: [
["t", "To-do"],
["p", "In-progress"],
["c", "Completed"],
],
}),
});

rerender(
<StringFilterValuePicker
query={query}
stageIndex={stageIndex}
column={column}
values={["c"]}
onChange={onChange}
/>,
);
const checkboxes = screen.getAllByRole("checkbox");
expect(checkboxes[0]).toHaveAccessibleName("In-progress");
expect(checkboxes[0]).not.toBeChecked();
expect(checkboxes[1]).toHaveAccessibleName("Completed");
expect(checkboxes[1]).toBeChecked();
expect(checkboxes[2]).toHaveAccessibleName("To-do");
expect(checkboxes[2]).not.toBeChecked();
});

it("should handle empty field values", async () => {
const { onChange, onFocus, onBlur } = await setupStringPicker({
query,
Expand Down Expand Up @@ -609,6 +702,31 @@ describe("NumberFilterValuePicker", () => {
userEvent.click(screen.getByText("In-progress"));
expect(onChange).toHaveBeenCalledWith([10, 20]);
});

it("should elevate selected field values on initial render", async () => {
await setupNumberPicker({
query,
stageIndex,
column,
values: [20],
fieldValues: createMockFieldValues({
field_id: ORDERS.QUANTITY,
values: [
[10, "To-do"],
[20, "In-progress"],
[30, "Completed"],
],
}),
});

const checkboxes = screen.getAllByRole("checkbox");
expect(checkboxes[0]).toHaveAccessibleName("In-progress");
expect(checkboxes[0]).toBeChecked();
expect(checkboxes[1]).toHaveAccessibleName("To-do");
expect(checkboxes[1]).not.toBeChecked();
expect(checkboxes[2]).toHaveAccessibleName("Completed");
expect(checkboxes[2]).not.toBeChecked();
});
});

describe("no values", () => {
Expand Down
19 changes: 12 additions & 7 deletions ...nd/src/metabase/querying/components/FilterValuePicker/ListValuePicker/ListValuePicker.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export function ListValuePicker({
}: ListValuePickerProps) {
if (!compact) {
return (
<DefaultValuePicker
<CheckboxListPicker
fieldValues={fieldValues}
selectedValues={selectedValues}
placeholder={placeholder}
Expand All @@ -49,7 +49,7 @@ export function ListValuePicker({

if (fieldValues.length <= MAX_INLINE_OPTIONS) {
return (
<CheckboxValuePicker
<CheckboxGridPicker
fieldValues={fieldValues}
selectedValues={selectedValues}
placeholder={placeholder}
Expand All @@ -59,7 +59,7 @@ export function ListValuePicker({
}

return (
<SelectValuePicker
<MultiSelectPicker
fieldValues={fieldValues}
selectedValues={selectedValues}
placeholder={placeholder}
Expand All @@ -68,15 +68,20 @@ export function ListValuePicker({
);
}

function DefaultValuePicker({
function CheckboxListPicker({
fieldValues,
selectedValues,
placeholder,
autoFocus,
onChange,
}: ListValuePickerProps) {
const [searchValue, setSearchValue] = useState("");
const options = getEffectiveOptions(fieldValues, selectedValues);
const [elevatedValues] = useState(selectedValues);
const options = getEffectiveOptions(
fieldValues,
selectedValues,
elevatedValues,
);
const visibleOptions = searchOptions(options, searchValue);

const handleInputChange = (event: ChangeEvent<HTMLInputElement>) => {
Expand Down Expand Up @@ -113,7 +118,7 @@ function DefaultValuePicker({
);
}

function CheckboxValuePicker({
function CheckboxGridPicker({
fieldValues,
selectedValues,
onChange,
Expand All @@ -140,7 +145,7 @@ function CheckboxValuePicker({
);
}

export function SelectValuePicker({
export function MultiSelectPicker({
fieldValues,
selectedValues,
placeholder,
Expand Down
15 changes: 10 additions & 5 deletions frontend/src/metabase/querying/components/FilterValuePicker/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,25 +41,30 @@ function getFieldOptions(fieldValues: FieldValue[]): SelectItem[] {
function getSelectedOptions(selectedValues: string[]): SelectItem[] {
return selectedValues.map(value => ({
value,
label: value,
}));
}

export function getEffectiveOptions(
fieldValues: FieldValue[],
selectedValues: string[],
elevatedValues: string[] = [],
): SelectItem[] {
const options = [
...getSelectedOptions(elevatedValues),
...getFieldOptions(fieldValues),
...getSelectedOptions(selectedValues),
];

const mapping = options.reduce((map: Record<string, string>, option) => {
map[option.value] ??= option.label ?? option.value;
const mapping = options.reduce((map: Map<string, string>, option) => {
if (option.label) {
map.set(option.value, option.label);
} else if (!map.has(option.value)) {
map.set(option.value, option.value);
}
return map;
}, {});
}, new Map<string, string>());

return Object.entries(mapping).map(([value, label]) => ({ value, label }));
return [...mapping.entries()].map(([value, label]) => ({ value, label }));
}

export function isKeyColumn(column: Lib.ColumnMetadata) {
Expand Down

0 comments on commit 697c1eb

Please sign in to comment.