Skip to content
Snippets Groups Projects
Unverified Commit ed7e147b authored by Alexander Polyankin's avatar Alexander Polyankin Committed by GitHub
Browse files

Add UI to swap clauses in the notebook (#39205) (#40131)

parent f73b40c5
No related branches found
No related tags found
No related merge requests found
Showing
with 320 additions and 83 deletions
......@@ -168,8 +168,11 @@ export const moveColumnDown = (column, distance) => {
.trigger("mouseup", 0, distance * 50, { force: true });
};
export const moveDnDKitColumnVertical = (column, distance) => {
column
export const moveDnDKitElement = (
element,
{ horizontal = 0, vertical = 0 } = {},
) => {
element
.trigger("pointerdown", 0, 0, {
force: true,
isPrimary: true,
......@@ -182,13 +185,13 @@ export const moveDnDKitColumnVertical = (column, distance) => {
button: 0,
})
.wait(200)
.trigger("pointermove", 0, distance, {
.trigger("pointermove", horizontal, vertical, {
force: true,
isPrimary: true,
button: 0,
})
.wait(200)
.trigger("pointerup", 0, distance, {
.trigger("pointerup", horizontal, vertical, {
force: true,
isPrimary: true,
button: 0,
......
......@@ -10,7 +10,7 @@ import {
saveDashboard,
getDashboardCardMenu,
getDraggableElements,
moveDnDKitColumnVertical,
moveDnDKitElement,
} from "e2e/support/helpers";
describe("scenarios > dashboard cards > visualization options", () => {
......@@ -51,7 +51,9 @@ describe("scenarios > dashboard cards > visualization options", () => {
getDashboardCard().realHover();
cy.findByLabelText("Show visualization options").click();
cy.findByTestId("chartsettings-sidebar").within(() => {
moveDnDKitColumnVertical(getDraggableElements().contains("ID"), 100);
moveDnDKitElement(getDraggableElements().contains("ID"), {
vertical: 100,
});
/**
* When this issue gets fixed, it should be safe to uncomment the following assertion.
......
......@@ -10,6 +10,7 @@ import {
getNotebookStep,
hovercard,
join,
moveDnDKitElement,
openNotebook,
openOrdersTable,
openProductsTable,
......@@ -708,6 +709,68 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => {
assertTableRowCount(10);
});
});
it("should be able to drag-n-drop query clauses", () => {
function moveElement({ name, horizontal, vertical, index }) {
moveDnDKitElement(cy.findByText(name), {
horizontal,
vertical,
});
cy.findAllByTestId("notebook-cell-item")
.eq(index)
.should("have.text", name);
}
const questionDetails = {
query: {
"source-table": ORDERS_ID,
expressions: {
E1: ["+", ["field", ORDERS.ID, null], 1],
E2: ["+", ["field", ORDERS.ID, null], 2],
},
filter: [
"and",
["=", ["field", ORDERS.ID, null], 1],
["=", ["field", ORDERS.ID, null], 2],
["=", ["field", ORDERS.ID, null], 3],
],
breakout: [
["field", ORDERS.ID, null],
["field", ORDERS.PRODUCT_ID, null],
],
aggregation: [
["count"],
["sum", ["field", ORDERS.TAX, null]],
["sum", ["field", ORDERS.SUBTOTAL, null]],
["sum", ["field", ORDERS.TOTAL, null]],
["avg", ["field", ORDERS.TOTAL, null]],
],
"order-by": [
["asc", ["aggregation", 0]],
["asc", ["aggregation", 4]],
],
},
};
cy.createQuestion(questionDetails, { visitQuestion: true });
openNotebook();
getNotebookStep("expression").within(() => {
moveElement({ name: "E1", horizontal: 100, index: 1 });
});
getNotebookStep("filter").within(() => {
moveElement({ name: "ID is 2", horizontal: -100, index: 0 });
});
getNotebookStep("summarize").within(() => {
cy.findByTestId("aggregate-step").within(() => {
moveElement({ name: "Count", vertical: 100, index: 3 });
});
cy.findByTestId("breakout-step").within(() => {
moveElement({ name: "ID", horizontal: 100, index: 1 });
});
});
getNotebookStep("sort").within(() => {
moveElement({ name: "Average of Total", horizontal: -100, index: 0 });
});
});
});
function assertTableRowCount(expectedCount) {
......
......@@ -9,7 +9,7 @@ import {
popover,
modal,
sidebar,
moveDnDKitColumnVertical,
moveDnDKitElement,
} from "e2e/support/helpers";
const { ORDERS, ORDERS_ID, PRODUCTS, PRODUCTS_ID } = SAMPLE_DATABASE;
......@@ -100,7 +100,7 @@ describe("scenarios > question > settings", () => {
getSidebarColumns().eq("5").as("total").contains("Total");
moveDnDKitColumnVertical(cy.get("@total"), -100);
moveDnDKitElement(cy.get("@total"), { vertical: -100 });
getSidebarColumns().eq("3").should("contain.text", "Total");
......@@ -114,7 +114,7 @@ describe("scenarios > question > settings", () => {
expect($el.scrollTop).to.eql(0);
});
moveDnDKitColumnVertical(cy.get("@title"), 15);
moveDnDKitElement(cy.get("@title"), { vertical: 15 });
cy.findByTestId("chartsettings-sidebar").should(([$el]) => {
expect($el.scrollTop).to.be.greaterThan(0);
......@@ -156,7 +156,7 @@ describe("scenarios > question > settings", () => {
.contains(/Products? → Category/);
// Drag and drop this column between "Tax" and "Discount" (index 5 in @sidebarColumns array)
moveDnDKitColumnVertical(cy.get("@prod-category"), -360);
moveDnDKitElement(cy.get("@prod-category"), { vertical: -360 });
refreshResultsInHeader();
......@@ -187,7 +187,7 @@ describe("scenarios > question > settings", () => {
findColumnAtIndex("User → Address", -1).as("user-address");
// Move it one place up
moveDnDKitColumnVertical(cy.get("@user-address"), -100);
moveDnDKitElement(cy.get("@user-address"), { vertical: -100 });
findColumnAtIndex("User → Address", -3);
......
......@@ -8,7 +8,7 @@ import {
popover,
visitDashboard,
cypressWaitAll,
moveDnDKitColumnVertical,
moveDnDKitElement,
} from "e2e/support/helpers";
const { ORDERS, ORDERS_ID, PEOPLE, PRODUCTS, PRODUCTS_ID } = SAMPLE_DATABASE;
......@@ -143,7 +143,7 @@ describe("scenarios > visualizations > bar chart", () => {
});
it("should allow you to show/hide and reorder columns", () => {
moveDnDKitColumnVertical(getDraggableElements().eq(0), 100);
moveDnDKitElement(getDraggableElements().eq(0), { vertical: 100 });
cy.findAllByTestId("legend-item").eq(0).should("contain.text", "Gadget");
cy.findAllByTestId("legend-item").eq(1).should("contain.text", "Gizmo");
......@@ -192,7 +192,7 @@ describe("scenarios > visualizations > bar chart", () => {
});
it("should gracefully handle removing filtered items, and adding new items to the end of the list", () => {
moveDnDKitColumnVertical(getDraggableElements().first(), 100);
moveDnDKitElement(getDraggableElements().first(), { vertical: 100 });
getDraggableElements()
.eq(1)
......
......@@ -6,7 +6,7 @@ import {
sidebar,
getDraggableElements,
popover,
moveDnDKitColumnVertical,
moveDnDKitElement,
} from "e2e/support/helpers";
const { PEOPLE_ID, PEOPLE } = SAMPLE_DATABASE;
......@@ -45,7 +45,7 @@ describe("scenarios > visualizations > funnel chart", () => {
.first()
.should("have.text", name);
moveDnDKitColumnVertical(getDraggableElements().first(), 100);
moveDnDKitElement(getDraggableElements().first(), { vertical: 100 });
getDraggableElements().eq(2).should("have.text", name);
......@@ -71,7 +71,7 @@ describe("scenarios > visualizations > funnel chart", () => {
});
it("should handle row items being filterd out and returned gracefully", () => {
moveDnDKitColumnVertical(getDraggableElements().first(), 100);
moveDnDKitElement(getDraggableElements().first(), { vertical: 100 });
getDraggableElements()
.eq(1)
......
......@@ -4,7 +4,7 @@ import {
restore,
visitQuestionAdhoc,
getDraggableElements,
moveDnDKitColumnVertical,
moveDnDKitElement,
} from "e2e/support/helpers";
const { ORDERS_ID, ORDERS } = SAMPLE_DATABASE;
......@@ -62,10 +62,9 @@ describe("issue 25250", () => {
cy.findByText("Product ID").should("be.visible");
cy.findByTestId("viz-settings-button").click();
moveDnDKitColumnVertical(
getDraggableElements().contains("Product ID"),
-100,
);
moveDnDKitElement(getDraggableElements().contains("Product ID"), {
vertical: -100,
});
getDraggableElements().eq(0).should("contain", "Product ID");
});
});
......@@ -17,7 +17,7 @@ import {
getTable,
leftSidebar,
sidebar,
moveDnDKitColumnVertical,
moveDnDKitElement,
} from "e2e/support/helpers";
describe("scenarios > visualizations > table", () => {
......@@ -431,10 +431,9 @@ describe("scenarios > visualizations > table > conditional formatting", () => {
.first()
.should("contain.text", "is less than 20");
moveDnDKitColumnVertical(
cy.findAllByTestId("formatting-rule-preview").eq(2),
-300,
);
moveDnDKitElement(cy.findAllByTestId("formatting-rule-preview").eq(2), {
vertical: -300,
});
cy.findAllByTestId("formatting-rule-preview")
.first()
......
......@@ -81,6 +81,15 @@ export function replaceClause(
return ML.replace_clause(query, stageIndex, targetClause, newClause);
}
export function swapClauses(
query: Query,
stageIndex: number,
sourceClause: Clause,
targetClause: Clause,
): Query {
return ML.swap_clauses(query, stageIndex, sourceClause, targetClause);
}
export function sourceTableOrCardId(query: Query): TableId | null {
return ML.source_table_or_card_id(query);
}
......
......@@ -65,6 +65,8 @@ export type OrderByDirection = "asc" | "desc";
declare const FilterClause: unique symbol;
export type FilterClause = unknown & { _opaque: typeof FilterClause };
export type Filterable = FilterClause | ExpressionClause | SegmentMetadata;
declare const Join: unique symbol;
export type Join = unknown & { _opaque: typeof Join };
......
......@@ -39,8 +39,21 @@ export function AggregateStep({
updateQuery(nextQuery);
};
const handleRemoveAggregation = (aggregation: Lib.AggregationClause) => {
const nextQuery = Lib.removeClause(query, stageIndex, aggregation);
const handleReorderAggregation = (
sourceClause: Lib.AggregationClause,
targetClause: Lib.AggregationClause,
) => {
const nextQuery = Lib.swapClauses(
query,
stageIndex,
sourceClause,
targetClause,
);
updateQuery(nextQuery);
};
const handleRemoveAggregation = (clause: Lib.AggregationClause) => {
const nextQuery = Lib.removeClause(query, stageIndex, clause);
updateQuery(nextQuery);
};
......@@ -66,6 +79,7 @@ export function AggregateStep({
onClose={onClose}
/>
)}
onReorder={handleReorderAggregation}
onRemove={handleRemoveAggregation}
data-testid="aggregate-step"
/>
......
......@@ -37,6 +37,19 @@ function BreakoutStep({
updateQuery(nextQuery);
};
const handleReorderBreakout = (
sourceClause: Lib.BreakoutClause,
targetClause: Lib.BreakoutClause,
) => {
const nextQuery = Lib.swapClauses(
query,
stageIndex,
sourceClause,
targetClause,
);
updateQuery(nextQuery);
};
const handleRemoveBreakout = (clause: Lib.BreakoutClause) => {
const nextQuery = Lib.removeClause(query, stageIndex, clause);
updateQuery(nextQuery);
......@@ -61,6 +74,7 @@ function BreakoutStep({
onClose={onClose}
/>
)}
onReorder={handleReorderBreakout}
onRemove={handleRemoveBreakout}
data-testid="breakout-step"
/>
......
import type { DndContextProps } from "@dnd-kit/core";
import { PointerSensor, useSensor, DndContext } from "@dnd-kit/core";
import { restrictToParentElement } from "@dnd-kit/modifiers";
import { SortableContext, useSortable } from "@dnd-kit/sortable";
import { CSS } from "@dnd-kit/utilities";
import type { ReactNode } from "react";
import { useCallback } from "react";
import { Icon } from "metabase/ui";
import {
......@@ -20,43 +28,35 @@ type RenderPopoverOpts<T> = {
onClose: () => void;
};
export interface ClauseStepProps<T> {
export type ClauseStepProps<T> = {
color: string;
items: T[];
isLastOpened?: boolean;
initialAddText?: string | null;
initialAddText?: string;
readOnly?: boolean;
isLastOpened?: boolean;
renderName: (item: T, index: number) => JSX.Element | string;
renderPopover: (opts: RenderPopoverOpts<T>) => JSX.Element | null;
canRemove?: (item: T) => boolean;
onRemove?: ((item: T, index: number) => void) | null;
onRemove: (item: T, index: number) => void;
onReorder: (sourceItem: T, targetItem: T) => void;
"data-testid"?: string;
}
};
export const ClauseStep = <T,>({
color,
items,
initialAddText,
readOnly = false,
isLastOpened = false,
initialAddText = null,
readOnly,
renderName,
renderPopover,
canRemove,
onRemove = null,
onRemove,
onReorder,
...props
}: ClauseStepProps<T>): JSX.Element => {
const renderNewItem = ({ onOpen }: { onOpen?: () => void }) => (
<NotebookCellAdd
initialAddText={items.length === 0 && initialAddText}
color={color}
onClick={onOpen}
/>
);
const renderItem = ({ item, index, onOpen }: RenderItemOpts<T>) => (
<NotebookCellItem color={color} readOnly={readOnly} onClick={onOpen}>
{renderName(item, index)}
{!readOnly && onRemove && (!canRemove || canRemove(item)) && (
{!readOnly && (
<Icon
className="ml1"
name="close"
......@@ -69,15 +69,26 @@ export const ClauseStep = <T,>({
</NotebookCellItem>
);
const renderNewItem = ({ onOpen }: { onOpen?: () => void }) => (
<NotebookCellAdd
initialAddText={items.length === 0 && initialAddText}
color={color}
onClick={onOpen}
/>
);
return (
<NotebookCell color={color} data-testid={props["data-testid"]}>
{items.map((item, index) => (
<ClausePopover
key={index}
renderItem={onOpen => renderItem({ item, index, onOpen })}
renderPopover={onClose => renderPopover({ item, index, onClose })}
/>
))}
<ClauseStepDndContext items={items} onReorder={onReorder}>
{items.map((item, index) => (
<ClauseStepDndItem key={index} index={index} readOnly={readOnly}>
<ClausePopover
renderItem={onOpen => renderItem({ item, index, onOpen })}
renderPopover={onClose => renderPopover({ item, index, onClose })}
/>
</ClauseStepDndItem>
))}
</ClauseStepDndContext>
{!readOnly && (
<ClausePopover
isInitiallyOpen={isLastOpened}
......@@ -88,3 +99,87 @@ export const ClauseStep = <T,>({
</NotebookCell>
);
};
type ClauseStepDndContextProps<T> = {
items: T[];
children: ReactNode;
onReorder: (sourceItem: T, targetItem: T) => void;
};
function ClauseStepDndContext<T>({
items,
children,
onReorder,
}: ClauseStepDndContextProps<T>) {
const pointerSensor = useSensor(PointerSensor, {
activationConstraint: { distance: 0 },
});
const handleSortEnd: DndContextProps["onDragEnd"] = useCallback(
input => {
if (input.over) {
const sourceIndex = getItemIndexFromId(input.active.id);
const targetIndex = getItemIndexFromId(input.over.id);
onReorder(items[sourceIndex], items[targetIndex]);
}
},
[items, onReorder],
);
return (
<DndContext
sensors={[pointerSensor]}
modifiers={[restrictToParentElement]}
onDragEnd={handleSortEnd}
>
<SortableContext
items={items.map((_, index) => getItemIdFromIndex(index))}
>
{children}
</SortableContext>
</DndContext>
);
}
type ClauseStepDndItemProps = {
index: number;
readOnly: boolean;
children: ReactNode;
};
function ClauseStepDndItem({
index,
readOnly,
children,
}: ClauseStepDndItemProps) {
const { attributes, listeners, setNodeRef, transform, transition } =
useSortable({
id: getItemIdFromIndex(index),
disabled: readOnly,
// disable animation after reordering because we don't have stable item ids
animateLayoutChanges: () => false,
});
return (
<div
ref={setNodeRef}
{...attributes}
{...listeners}
style={{
transition,
transform: CSS.Translate.toString(transform),
}}
>
{children}
</div>
);
}
// dnd-kit ignores `0` item, so we convert indexes to string `"0"`
function getItemIdFromIndex(index: number) {
return String(index);
}
function getItemIndexFromId(id: string | number) {
return Number(id);
}
......@@ -17,8 +17,27 @@ export const ExpressionStep = ({
const { query, stageIndex } = step;
const expressions = Lib.expressions(query, stageIndex);
const renderExpressionName = (expression: Lib.ExpressionClause) =>
Lib.displayInfo(query, stageIndex, expression).longDisplayName;
const renderExpressionName = (expression: Lib.ExpressionClause) => {
return Lib.displayInfo(query, stageIndex, expression).longDisplayName;
};
const handleReorderExpression = (
sourceClause: Lib.ExpressionClause,
targetClause: Lib.ExpressionClause,
) => {
const nextQuery = Lib.swapClauses(
query,
stageIndex,
sourceClause,
targetClause,
);
updateQuery(nextQuery);
};
const handleRemoveExpression = (clause: Lib.ExpressionClause) => {
const nextQuery = Lib.removeClause(query, stageIndex, clause);
updateQuery(nextQuery);
};
return (
<ClauseStep
......@@ -71,10 +90,8 @@ export const ExpressionStep = ({
/>
)}
isLastOpened={isLastOpened}
onRemove={clause => {
const nextQuery = Lib.removeClause(query, stageIndex, clause);
updateQuery(nextQuery);
}}
onReorder={handleReorderExpression}
onRemove={handleRemoveExpression}
/>
);
};
......
......@@ -23,33 +23,44 @@ export function FilterStep({
[query, stageIndex],
);
const handleAddFilter = (
filter: Lib.ExpressionClause | Lib.FilterClause | Lib.SegmentMetadata,
) => {
const nextQuery = Lib.filter(query, stageIndex, filter);
const renderFilterName = (filter: Lib.FilterClause) =>
Lib.displayInfo(query, stageIndex, filter).longDisplayName;
const handleAddFilter = (clause: Lib.Filterable) => {
const nextQuery = Lib.filter(query, stageIndex, clause);
updateQuery(nextQuery);
};
const handleUpdateFilter = (
targetFilter: Lib.FilterClause,
nextFilter: Lib.ExpressionClause | Lib.FilterClause | Lib.SegmentMetadata,
targetClause: Lib.FilterClause,
newClause: Lib.Filterable,
) => {
const nextQuery = Lib.replaceClause(
query,
stageIndex,
targetFilter,
nextFilter,
targetClause,
newClause,
);
updateQuery(nextQuery);
};
const handleRemoveFilter = (filter: Lib.FilterClause) => {
const nextQuery = Lib.removeClause(query, stageIndex, filter);
const handleReorderFilter = (
sourceClause: Lib.FilterClause,
targetClause: Lib.FilterClause,
) => {
const nextQuery = Lib.swapClauses(
query,
stageIndex,
sourceClause,
targetClause,
);
updateQuery(nextQuery);
};
const renderFilterName = (filter: Lib.FilterClause) =>
Lib.displayInfo(query, stageIndex, filter).longDisplayName;
const handleRemoveFilter = (clause: Lib.FilterClause) => {
const nextQuery = Lib.removeClause(query, stageIndex, clause);
updateQuery(nextQuery);
};
return (
<ErrorBoundary>
......@@ -71,6 +82,7 @@ export function FilterStep({
onClose={onClose}
/>
)}
onReorder={handleReorderFilter}
onRemove={handleRemoveFilter}
/>
</ErrorBoundary>
......@@ -82,12 +94,10 @@ interface FilterPopoverProps {
stageIndex: number;
filter?: Lib.FilterClause;
filterIndex?: number;
onAddFilter: (
filter: Lib.ExpressionClause | Lib.FilterClause | Lib.SegmentMetadata,
) => void;
onAddFilter: (filter: Lib.Filterable) => void;
onUpdateFilter: (
targetFilter: Lib.FilterClause,
nextFilter: Lib.ExpressionClause | Lib.FilterClause | Lib.SegmentMetadata,
nextFilter: Lib.Filterable,
) => void;
onClose?: () => void;
}
......
......@@ -43,6 +43,19 @@ function SortStep({
updateQuery(nextQuery);
};
const handleReorderOrderBy = (
sourceClause: Lib.OrderByClause,
targetClause: Lib.OrderByClause,
) => {
const nextQuery = Lib.swapClauses(
query,
stageIndex,
sourceClause,
targetClause,
);
updateQuery(nextQuery);
};
const handleRemoveOrderBy = (clause: Lib.OrderByClause) => {
const nextQuery = Lib.removeClause(query, stageIndex, clause);
updateQuery(nextQuery);
......@@ -71,6 +84,7 @@ function SortStep({
onClose={onClose}
/>
)}
onReorder={handleReorderOrderBy}
onRemove={handleRemoveOrderBy}
/>
);
......
......@@ -15,9 +15,7 @@ export interface FilterPickerProps {
filter?: Lib.FilterClause;
filterIndex?: number;
onSelect: (
filter: Lib.ExpressionClause | Lib.FilterClause | Lib.SegmentMetadata,
) => void;
onSelect: (filter: Lib.Filterable) => void;
onClose?: () => void;
}
......@@ -46,9 +44,7 @@ export function FilterPicker({
setFilter(initialFilter);
}, [initialFilter]);
const handleChange = (
filter: Lib.ExpressionClause | Lib.FilterClause | Lib.SegmentMetadata,
) => {
const handleChange = (filter: Lib.Filterable) => {
onSelect(filter);
onClose?.();
};
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment