From 1284079b7e5ac6476e7e720cdee592dfad0626d9 Mon Sep 17 00:00:00 2001 From: Nick Fitzpatrick <nick@metabase.com> Date: Mon, 12 Sep 2022 13:42:29 -0300 Subject: [PATCH] 24840 reordering breakouts (#25318) --- .../components/ChartSettings.jsx | 9 ++- .../components/FunnelNormal.jsx | 2 +- .../components/LineAreaBarChart.jsx | 23 ++++-- .../settings/ChartSettingFieldsPicker.jsx | 7 +- .../ChartSettingFieldsPicker.styled.tsx | 9 +++ .../settings/ChartSettingOrderedRows.tsx | 71 ------------------- ...x => ChartSettingOrderedSimple.styled.tsx} | 6 +- .../settings/ChartSettingOrderedSimple.tsx | 68 ++++++++++++++++++ .../components/settings/ColumnItem.jsx | 6 +- .../components/settings/ColumnItem.styled.tsx | 8 ++- .../visualizations/lib/settings/graph.js | 44 +++++++++++- .../visualizations/visualizations/Funnel.jsx | 17 ++--- .../visualizaiton-options.cy.spec.js | 4 +- .../native/reproductions/17060.cy.spec.js | 2 +- .../reproductions/17514-ui-overlay.cy.spec.js | 2 +- .../scenarios/question/settings.cy.spec.js | 4 +- 16 files changed, 181 insertions(+), 101 deletions(-) create mode 100644 frontend/src/metabase/visualizations/components/settings/ChartSettingFieldsPicker.styled.tsx delete mode 100644 frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedRows.tsx rename frontend/src/metabase/visualizations/components/settings/{ChartSettingOrderedRows.styled.tsx => ChartSettingOrderedSimple.styled.tsx} (69%) create mode 100644 frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedSimple.tsx diff --git a/frontend/src/metabase/visualizations/components/ChartSettings.jsx b/frontend/src/metabase/visualizations/components/ChartSettings.jsx index 1388979bbe2..96fbe5a427c 100644 --- a/frontend/src/metabase/visualizations/components/ChartSettings.jsx +++ b/frontend/src/metabase/visualizations/components/ChartSettings.jsx @@ -185,6 +185,7 @@ class ChartSettings extends Component { children, setSidebarPropsOverride, dashboard, + isDashboard, } = this.props; const { currentWidget, popoverRef } = this.state; @@ -307,7 +308,13 @@ class ChartSettings extends Component { return ( <div className={cx(className, "flex flex-column")}> {showSectionPicker && ( - <div className="flex flex-no-shrink pl4 pb1">{sectionPicker}</div> + <div + className={cx("flex flex-no-shrink pl4 pb1", { + pt3: isDashboard, + })} + > + {sectionPicker} + </div> )} {noPreview ? ( <div className="full-height relative scroll-y scroll-show pt2 pb4"> diff --git a/frontend/src/metabase/visualizations/components/FunnelNormal.jsx b/frontend/src/metabase/visualizations/components/FunnelNormal.jsx index 842509d7cb5..9a495775074 100644 --- a/frontend/src/metabase/visualizations/components/FunnelNormal.jsx +++ b/frontend/src/metabase/visualizations/components/FunnelNormal.jsx @@ -29,7 +29,7 @@ export default class FunnelNormal extends Component { const rows = settings["funnel.rows"] ? settings["funnel.rows"] .filter(fr => fr.enabled) - .map(fr => series[fr.rowIndex].data.rows[0]) + .map(fr => series[fr.originalIndex].data.rows[0]) : series.map(s => s.data.rows[0]); const isNarrow = gridSize && gridSize.width < 7; diff --git a/frontend/src/metabase/visualizations/components/LineAreaBarChart.jsx b/frontend/src/metabase/visualizations/components/LineAreaBarChart.jsx index 53ee08cbd20..1b3dd39d6e6 100644 --- a/frontend/src/metabase/visualizations/components/LineAreaBarChart.jsx +++ b/frontend/src/metabase/visualizations/components/LineAreaBarChart.jsx @@ -117,6 +117,14 @@ export default class LineAreaBarChart extends Component { t`Choose fields`, ); } + const seriesOrder = (settings["graph.series_order"] || []).filter( + series => series.enabled, + ); + if (dimensions.length > 1 && seriesOrder.length === 0) { + throw new ChartSettingsError(t`No breakouts are enabled`, { + section: t`Data`, + }); + } } static seriesAreCompatible(initialSeries, newSeries) { @@ -263,10 +271,9 @@ export default class LineAreaBarChart extends Component { return settings; } - getLegendSettings() { + getLegendSettings(series) { const { card, - series, settings, showTitle, actionButtons, @@ -361,8 +368,16 @@ export default class LineAreaBarChart extends Component { onHoverChange, onAddSeries, onRemoveSeries, + settings, } = this.props; + const orderedSeries = + (settings["graph.dimensions"]?.length > 1 && + settings["graph.series_order"] + ?.filter(orderedItem => orderedItem.enabled) + .map(orderedItem => series[orderedItem.originalIndex])) || + series; + const { title, description, @@ -372,7 +387,7 @@ export default class LineAreaBarChart extends Component { hasLegend, hasBreakout, canSelectTitle, - } = this.getLegendSettings(); + } = this.getLegendSettings(orderedSeries); return ( <LineAreaBarChartRoot @@ -407,7 +422,7 @@ export default class LineAreaBarChart extends Component { > <CardRenderer {...this.props} - series={series} + series={orderedSeries} settings={this.getSettings()} className="renderer flex-full" maxSeries={MAX_SERIES} diff --git a/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldsPicker.jsx b/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldsPicker.jsx index ffe060b8fa1..2e156e84f09 100644 --- a/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldsPicker.jsx +++ b/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldsPicker.jsx @@ -2,6 +2,7 @@ import React from "react"; import { t } from "ttag"; import ChartSettingFieldPicker from "./ChartSettingFieldPicker"; +import { AddAnotherContainer } from "./ChartSettingFieldsPicker.styled"; const ChartSettingFieldsPicker = ({ value = [], @@ -46,9 +47,9 @@ const ChartSettingFieldsPicker = ({ <span className="text-error">{t`error`}</span> )} {addAnother && ( - <div className="mt2 mb3"> + <AddAnotherContainer> <a - className="text-brand text-bold py1 px2 rounded bg-light bg-medium-hover" + className="text-brand text-bold py1" onClick={() => { const remaining = options.filter(o => value.indexOf(o.value) < 0); if (remaining.length === 1) { @@ -62,7 +63,7 @@ const ChartSettingFieldsPicker = ({ > {addAnother} </a> - </div> + </AddAnotherContainer> )} </div> ); diff --git a/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldsPicker.styled.tsx b/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldsPicker.styled.tsx new file mode 100644 index 00000000000..6f92ba04b75 --- /dev/null +++ b/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldsPicker.styled.tsx @@ -0,0 +1,9 @@ +import styled from "@emotion/styled"; +import { color } from "metabase/lib/colors"; + +export const AddAnotherContainer = styled.div` + margin-top: 1rem; + margin-bottom: 1rem; + padding-bottom: 1rem; + border-bottom: 1px solid ${color("border")}; +`; diff --git a/frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedRows.tsx b/frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedRows.tsx deleted file mode 100644 index f572a148b27..00000000000 --- a/frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedRows.tsx +++ /dev/null @@ -1,71 +0,0 @@ -import { updateIn } from "icepick"; -import React from "react"; -import { t } from "ttag"; - -import { ChartSettingOrderedItems } from "./ChartSettingOrderedItems"; - -import { - ChartSettingMessage, - ChartSettingOrderedRowsRoot, -} from "./ChartSettingOrderedRows.styled"; - -interface Row { - enabled: boolean; - rowIndex: number; - name: string; -} - -interface ChartSettingOrderedRowsProps { - onChange: (rows: Row[]) => void; - rows: Row[]; - value: Row[]; -} - -export const ChartSettingOrderedRows = ({ - onChange, - rows, - value: orderedRows, -}: ChartSettingOrderedRowsProps) => { - const handleDisable = (row: Row) => { - const index = orderedRows.findIndex(r => r.rowIndex === row.rowIndex); - onChange( - updateIn(orderedRows, [index], row => ({ - ...row, - enabled: !row.enabled, - })), - ); - }; - - const handleSortEnd = ({ - oldIndex, - newIndex, - }: { - oldIndex: number; - newIndex: number; - }) => { - const rowsCopy = [...orderedRows]; - rowsCopy.splice(newIndex, 0, rowsCopy.splice(oldIndex, 1)[0]); - onChange(rowsCopy); - }; - - const getRowTitle = (row: Row) => { - return rows.find(r => r.rowIndex === row.rowIndex)?.name || "Unknown"; - }; - - return ( - <ChartSettingOrderedRowsRoot> - {orderedRows.length > 0 ? ( - <ChartSettingOrderedItems - items={orderedRows} - getItemName={getRowTitle} - onRemove={handleDisable} - onEnable={handleDisable} - onSortEnd={handleSortEnd} - distance={5} - /> - ) : ( - <ChartSettingMessage>{t`Nothing to order`}</ChartSettingMessage> - )} - </ChartSettingOrderedRowsRoot> - ); -}; diff --git a/frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedRows.styled.tsx b/frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedSimple.styled.tsx similarity index 69% rename from frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedRows.styled.tsx rename to frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedSimple.styled.tsx index a6f4691d331..31f0f9103aa 100644 --- a/frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedRows.styled.tsx +++ b/frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedSimple.styled.tsx @@ -1,8 +1,10 @@ import styled from "@emotion/styled"; import { color } from "metabase/lib/colors"; -export const ChartSettingOrderedRowsRoot = styled.div` - margin-left: 1rem; +export const ChartSettingOrderedSimpleRoot = styled.div` + padding-left: 1rem; + padding-bottom: 0.5rem; + border-bottom: 1px solid ${color("border")}; `; export const ChartSettingMessage = styled.div` diff --git a/frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedSimple.tsx b/frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedSimple.tsx new file mode 100644 index 00000000000..5b402c15ed3 --- /dev/null +++ b/frontend/src/metabase/visualizations/components/settings/ChartSettingOrderedSimple.tsx @@ -0,0 +1,68 @@ +import { updateIn } from "icepick"; +import React from "react"; +import { t } from "ttag"; + +import { ChartSettingOrderedItems } from "./ChartSettingOrderedItems"; + +import { + ChartSettingMessage, + ChartSettingOrderedSimpleRoot, +} from "./ChartSettingOrderedSimple.styled"; + +interface SortableItem { + enabled: boolean; + originalIndex: number; + name: string; +} + +interface ChartSettingOrderedSimpleProps { + onChange: (rows: SortableItem[]) => void; + items: SortableItem[]; + value: SortableItem[]; +} + +export const ChartSettingOrderedSimple = ({ + onChange, + items, + value: orderedItems, +}: ChartSettingOrderedSimpleProps) => { + const toggleDisplay = (selectedItem: SortableItem) => { + const index = orderedItems.findIndex( + item => item.originalIndex === selectedItem.originalIndex, + ); + onChange(updateIn(orderedItems, [index, "enabled"], enabled => !enabled)); + }; + + const handleSortEnd = ({ + oldIndex, + newIndex, + }: { + oldIndex: number; + newIndex: number; + }) => { + const itemsCopy = [...orderedItems]; + itemsCopy.splice(newIndex, 0, itemsCopy.splice(oldIndex, 1)[0]); + onChange(itemsCopy); + }; + + const getItemTitle = (item: SortableItem) => { + return items[item.originalIndex]?.name || "Unknown"; + }; + + return ( + <ChartSettingOrderedSimpleRoot> + {orderedItems.length > 0 ? ( + <ChartSettingOrderedItems + items={orderedItems} + getItemName={getItemTitle} + onRemove={toggleDisplay} + onEnable={toggleDisplay} + onSortEnd={handleSortEnd} + distance={5} + /> + ) : ( + <ChartSettingMessage>{t`Nothing to order`}</ChartSettingMessage> + )} + </ChartSettingOrderedSimpleRoot> + ); +}; diff --git a/frontend/src/metabase/visualizations/components/settings/ColumnItem.jsx b/frontend/src/metabase/visualizations/components/settings/ColumnItem.jsx index eacf0243b92..82b56cf5d8e 100644 --- a/frontend/src/metabase/visualizations/components/settings/ColumnItem.jsx +++ b/frontend/src/metabase/visualizations/components/settings/ColumnItem.jsx @@ -30,7 +30,11 @@ const ColumnItem = ({ draggable, }) => { return ( - <ColumnItemRoot draggable={draggable} onClick={onClick}> + <ColumnItemRoot + onClick={onClick} + isDraggable={draggable} + data-testid={`draggable-item-${title}`} + > <ColumnItemContainer> {draggable && <ColumnItemDragHandle name="grabber2" size={12} />} <ColumnItemContent> diff --git a/frontend/src/metabase/visualizations/components/settings/ColumnItem.styled.tsx b/frontend/src/metabase/visualizations/components/settings/ColumnItem.styled.tsx index a6b4c7c0680..a6b77b6e04a 100644 --- a/frontend/src/metabase/visualizations/components/settings/ColumnItem.styled.tsx +++ b/frontend/src/metabase/visualizations/components/settings/ColumnItem.styled.tsx @@ -3,7 +3,11 @@ import { color } from "metabase/lib/colors"; import Icon from "metabase/components/Icon"; -export const ColumnItemRoot = styled.div` +interface ColumnItemRootProps { + isDraggable: boolean; +} + +export const ColumnItemRoot = styled.div<ColumnItemRootProps>` margin: 0.5rem 0; overflow: hidden; display: flex; @@ -18,7 +22,7 @@ export const ColumnItemRoot = styled.div` } ${props => - props.draggable && + props.isDraggable && ` cursor: grab; &:hover { diff --git a/frontend/src/metabase/visualizations/lib/settings/graph.js b/frontend/src/metabase/visualizations/lib/settings/graph.js index aa4730c8a3f..41e38aff59e 100644 --- a/frontend/src/metabase/visualizations/lib/settings/graph.js +++ b/frontend/src/metabase/visualizations/lib/settings/graph.js @@ -12,7 +12,10 @@ import { preserveExistingColumnsOrder, } from "metabase/visualizations/lib/utils"; -import { seriesSetting } from "metabase/visualizations/lib/settings/series"; +import { + seriesSetting, + keyForSingleSeries, +} from "metabase/visualizations/lib/settings/series"; import { columnSettings } from "metabase/visualizations/lib/settings/column"; import { getOptionFromColumn } from "metabase/visualizations/lib/settings/utils"; @@ -22,6 +25,8 @@ import { dimensionIsTimeseries } from "metabase/visualizations/lib/timeseries"; import _ from "underscore"; import { getMaxMetricsSupported } from "metabase/visualizations"; +import { ChartSettingOrderedSimple } from "metabase/visualizations/components/settings/ChartSettingOrderedSimple"; + // NOTE: currently we don't consider any date extracts to be histgrams const HISTOGRAM_DATE_EXTRACTS = new Set([ // "minute-of-hour", @@ -122,7 +127,7 @@ export const GRAPH_DATA_SETTINGS = { options.length > value.length && value.length < 2 && vizSettings["graph.metrics"].length < 2 - ? t`Add a series breakout...` + ? t`Add series breakout` : null, columns: data.cols, showColumnSetting: true, @@ -133,6 +138,41 @@ export const GRAPH_DATA_SETTINGS = { dashboard: false, useRawSeries: true, }, + "graph.series_order": { + section: t`Data`, + widget: ChartSettingOrderedSimple, + marginBottom: "1rem", + isValid: (series, settings) => { + const seriesOrder = settings["graph.series_order"]; + + if (!seriesOrder || !_.isArray(seriesOrder)) { + return false; + } + + return seriesOrder.length === series.length; + }, + getDefault: series => { + const keys = series.map(s => keyForSingleSeries(s)); + return keys.map((key, index) => ({ + name: key, + originalIndex: index, + enabled: true, + })); + }, + getProps: (series, settings) => { + const seriesSettings = settings["series_settings"] || {}; + const keys = series.map(s => keyForSingleSeries(s)); + return { + items: keys.map((key, index) => ({ + name: seriesSettings[key]?.title || key, + originalIndex: index, + })), + }; + }, + getHidden: (series, settings) => { + return settings["graph.dimensions"]?.length < 2 || series.length > 20; + }, + }, "graph.metrics": { section: t`Data`, title: t`Y-axis`, diff --git a/frontend/src/metabase/visualizations/visualizations/Funnel.jsx b/frontend/src/metabase/visualizations/visualizations/Funnel.jsx index bc512b4c1e8..4c80c739bb7 100644 --- a/frontend/src/metabase/visualizations/visualizations/Funnel.jsx +++ b/frontend/src/metabase/visualizations/visualizations/Funnel.jsx @@ -26,7 +26,7 @@ import _ from "underscore"; import cx from "classnames"; import ChartCaption from "metabase/visualizations/components/ChartCaption"; -import { ChartSettingOrderedRows } from "metabase/visualizations/components/settings/ChartSettingOrderedRows"; +import { ChartSettingOrderedSimple } from "metabase/visualizations/components/settings/ChartSettingOrderedSimple"; const propTypes = { headerIcon: PropTypes.shape(iconPropTypes), @@ -85,7 +85,7 @@ export default class Funnel extends Component { "funnel.dimension": "Total Sessions", }, dataset_query: { type: "null" }, - rowIndex: index, + originalIndex: index, }, data: { rows: [row], @@ -114,19 +114,20 @@ export default class Funnel extends Component { }), "funnel.rows": { section: t`Data`, - widget: ChartSettingOrderedRows, + widget: ChartSettingOrderedSimple, isValid: (series, settings) => { + console.log(series); const funnelRows = settings["funnel.rows"]; if (!funnelRows || !_.isArray(funnelRows)) { return false; } - if (!funnelRows.every(setting => setting.rowIndex !== undefined)) { + if (!funnelRows.every(setting => setting.originalIndex !== undefined)) { return false; } return ( - funnelRows.every(setting => series[setting.rowIndex]) && + funnelRows.every(setting => series[setting.originalIndex]) && funnelRows.length === series.length ); }, @@ -134,12 +135,12 @@ export default class Funnel extends Component { getDefault: transformedSeries => { return transformedSeries.map(s => ({ name: s.card.name, - rowIndex: s.card.rowIndex, + originalIndex: s.card.originalIndex, enabled: true, })); }, getProps: transformedSeries => ({ - rows: transformedSeries.map(s => s.card), + items: transformedSeries.map(s => s.card), }), }, ...metricSetting("funnel.metric", { @@ -197,7 +198,7 @@ export default class Funnel extends Component { name: formatValue(row[dimensionIndex], { column: cols[dimensionIndex], }), - rowIndex: index, + originalIndex: index, _transformed: true, }, data: { diff --git a/frontend/test/metabase/scenarios/dashboard/visualizaiton-options.cy.spec.js b/frontend/test/metabase/scenarios/dashboard/visualizaiton-options.cy.spec.js index f297b47d88e..b33cfbe135f 100644 --- a/frontend/test/metabase/scenarios/dashboard/visualizaiton-options.cy.spec.js +++ b/frontend/test/metabase/scenarios/dashboard/visualizaiton-options.cy.spec.js @@ -13,7 +13,7 @@ describe("scenarios > dashboard > visualization options", () => { cy.icon("palette").click(); cy.findByTestId("chartsettings-sidebar").within(() => { cy.findByText("ID") - .closest("[draggable=true]") + .closest("[data-testid^=draggable-item]") .trigger("mousedown", 0, 0, { force: true }) .trigger("mousemove", 5, 5, { force: true }) .trigger("mousemove", 0, 100, { force: true }) @@ -24,7 +24,7 @@ describe("scenarios > dashboard > visualization options", () => { * It currently doesn't work in UI at all, but Cypress somehow manages to move the "ID" column. * However, it leaves an empty column in its place (thus, making it impossible to use this assertion). */ - cy.get("[draggable=true]") + cy.findAllByTestId(/draggable-item/) .as("sidebarColumns") // Out of all the columns in the sidebar... .first() // ...pick the fist one and make sure it's not "ID" anymore .should("contain", "User ID"); diff --git a/frontend/test/metabase/scenarios/native/reproductions/17060.cy.spec.js b/frontend/test/metabase/scenarios/native/reproductions/17060.cy.spec.js index 42dceee6e01..2a821479d47 100644 --- a/frontend/test/metabase/scenarios/native/reproductions/17060.cy.spec.js +++ b/frontend/test/metabase/scenarios/native/reproductions/17060.cy.spec.js @@ -50,7 +50,7 @@ describe("issue 17060", () => { }); function rearrangeColumns() { - cy.get("[draggable=true]") + cy.findAllByTestId(/draggable-item/) .first() .trigger("mousedown", 0, 0, { force: true }) .trigger("mousemove", 5, 5, { force: true }) diff --git a/frontend/test/metabase/scenarios/question/reproductions/17514-ui-overlay.cy.spec.js b/frontend/test/metabase/scenarios/question/reproductions/17514-ui-overlay.cy.spec.js index be68e95fb1f..96557587827 100644 --- a/frontend/test/metabase/scenarios/question/reproductions/17514-ui-overlay.cy.spec.js +++ b/frontend/test/metabase/scenarios/question/reproductions/17514-ui-overlay.cy.spec.js @@ -181,7 +181,7 @@ function moveColumnToTop(column) { cy.findByTestId("sidebar-left").within(() => { cy.findByText(column) .should("be.visible") - .closest("[draggable=true]") + .closest("[data-testid^=draggable-item]") .trigger("mousedown", 0, 0, { force: true }) .trigger("mousemove", 5, 5, { force: true }) .trigger("mousemove", 0, -600, { force: true }) diff --git a/frontend/test/metabase/scenarios/question/settings.cy.spec.js b/frontend/test/metabase/scenarios/question/settings.cy.spec.js index 3ebc9aa71a6..c9732bd996f 100644 --- a/frontend/test/metabase/scenarios/question/settings.cy.spec.js +++ b/frontend/test/metabase/scenarios/question/settings.cy.spec.js @@ -116,7 +116,7 @@ describe("scenarios > question > settings", () => { // Remove "Total" getSidebarColumns() .contains("Total") - .closest("[draggable=true]") + .closest("[data-testid^=draggable-item]") .find(".Icon-eye_filled") .click(); @@ -267,5 +267,5 @@ function getSidebarColumns() { .scrollIntoView() .should("be.visible") .parent() - .find("[draggable=true]"); + .find("[data-testid^=draggable-item]"); } -- GitLab