From c7e270d32bbd2185dacce839a87de452e1daae6d Mon Sep 17 00:00:00 2001 From: Paul Rosenzweig <paulrosenzweig@users.noreply.github.com> Date: Tue, 3 Sep 2019 16:16:01 -0400 Subject: [PATCH] Refactor apply_tooltips (#10555) --- .../visualizations/lib/apply_tooltips.js | 474 +++++++----------- .../visualizations/visualizations/Scalar.jsx | 7 +- .../__support__/visualizations.js | 2 + .../LineAreaBarRenderer-bar.unit.spec.js | 24 +- .../lib/apply_tooltips.unit.spec.js | 132 +++++ 5 files changed, 355 insertions(+), 284 deletions(-) create mode 100644 frontend/test/metabase/visualizations/lib/apply_tooltips.unit.spec.js diff --git a/frontend/src/metabase/visualizations/lib/apply_tooltips.js b/frontend/src/metabase/visualizations/lib/apply_tooltips.js index 784902b7649..4403fb90d94 100644 --- a/frontend/src/metabase/visualizations/lib/apply_tooltips.js +++ b/frontend/src/metabase/visualizations/lib/apply_tooltips.js @@ -1,122 +1,228 @@ /// code to "apply" chart tooltips. (How does one apply a tooltip?) -import _ from "underscore"; import d3 from "d3"; +import moment from "moment"; import { formatValue } from "metabase/lib/formatting"; -import type { ClickObject } from "metabase/meta/types/Visualization"; import { isNormalized, isStacked } from "./renderer_utils"; import { determineSeriesIndexFromElement } from "./tooltip"; import { getFriendlyName } from "./utils"; -function clickObjectFromEvent(d, { series, isStacked, isScalarSeries }) { - let [ - { - data: { cols }, - }, - ] = series; - const seriesIndex = determineSeriesIndexFromElement(this, isStacked); - const card = series[seriesIndex].card; - const isSingleSeriesBar = - this.classList.contains("bar") && series.length === 1; +export function getClickHoverObject( + d, + { series, isNormalized, seriesIndex, seriesTitle, classList, event, element }, +) { + let { cols } = series[0].data; + const { card } = series[seriesIndex]; + + const isMultiseries = series.length > 1; + const isBreakoutMultiseries = isMultiseries && card._breakoutColumn; + const isBar = classList.includes("bar"); + const isSingleSeriesBar = isBar && !isMultiseries; - let clicked: ?ClickObject; + // always format the second column as the series name? + function getColumnDisplayName(col) { + // don't replace with series title for breakout multiseries since the series title is shown in the breakout value + if (col === cols[1] && !isBreakoutMultiseries && seriesTitle) { + return seriesTitle; + } else { + return getFriendlyName(col); + } + } + + let data = []; + let dimensions = []; + let value; if (Array.isArray(d.key)) { + value = d.key[2]; // scatter - clicked = { - value: d.key[2], - column: cols[2], - dimensions: [ - { value: d.key[0], column: cols[0] }, - { value: d.key[1], column: cols[1] }, - ].filter( - ({ column }) => - // don't include aggregations since we can't filter on them - column.source !== "aggregation", - ), - origin: d.key._origin, - }; - } else if (isScalarSeries) { - // special case for multi-series scalar series, which should be treated as scalars - clicked = { - value: d.data.value, - column: series[seriesIndex].data.cols[1], - }; + if (d.key._origin) { + data = d.key._origin.row.map((value, index) => { + const col = d.key._origin.cols[index]; + return { + key: getColumnDisplayName(col), + value: value, + col, + }; + }); + } else { + data = d.key.map((value, index) => ({ + key: getColumnDisplayName(cols[index]), + value: value, + col: cols[index], + })); + } + dimensions = [ + { value: d.key[0], column: cols[0] }, + { value: d.key[1], column: cols[1] }, + ]; + if (isBreakoutMultiseries) { + const { _breakoutValue: value, _breakoutColumn: column } = card; + dimensions.push({ value, column }); + } } else if (d.data) { + ({ value } = d.data); // line, area, bar if (!isSingleSeriesBar) { cols = series[seriesIndex].data.cols; } - clicked = { - value: d.data.value, - column: cols[1], - dimensions: [{ value: d.data.key, column: cols[0] }], - }; - } else { - clicked = { - dimensions: [], - }; - } - // handle multiseries - if (clicked && series.length > 1) { - if (card._breakoutColumn) { - // $FlowFixMe - clicked.dimensions.push({ - value: card._breakoutValue, - column: card._breakoutColumn, + const seriesData = series[seriesIndex].data || {}; + const rawCols = seriesData._rawCols || cols; + const { key } = d.data; + + // We look through the rows to match up they key in d.data to the x value + // from some row. + const row = seriesData.rows.find(([x]) => + moment.isMoment(key) + ? // for dates, we check two things: + // 1. does parsing x produce an equivalent moment value? + key.isSame(moment(x)) || + // 2. if not, we format the key using the column info + // this catches values like years that don't parse correctly above + formatValue(key, { column: rawCols[0] }) === String(x) + : // otherwise, we just check if the string value matches + // e.g. String("123") === String(123) + String(x) === String(key), + ); + + // try to get row from _origin but fall back to the row we already have + const rawRow = (row && row._origin && row._origin.row) || row; + + // Loop over *all* of the columns and create the new array + if (rawRow) { + data = rawCols.map((col, i) => { + if (isNormalized && cols[1].field_ref === col.field_ref) { + return { + key: getColumnDisplayName(cols[1]), + value: formatValue(d.data.value, { + number_style: "percent", + column: cols[1], + decimals: cols[1].decimals, + }), + col: col, + }; + } + return { + key: getColumnDisplayName(col), + value: rawRow[i], + col: col, + }; }); } + dimensions = rawCols.map((column, i) => ({ column, value: rawRow[i] })); + } else if (isBreakoutMultiseries) { + // an area doesn't have any data, but might have a breakout series to show + const { _breakoutValue: value, _breakoutColumn: column } = card; + data = [{ key: getColumnDisplayName(column), col: column, value }]; + dimensions = [{ column, value }]; } - if (card._seriesIndex != null) { - // $FlowFixMe - clicked.seriesIndex = card._seriesIndex; - } + // overwrite value/col for breakout column + data = data.map(d => + d.col === card._breakoutColumn + ? { + ...d, + // Use series title if it's set + value: seriesTitle ? seriesTitle : card._breakoutValue, + // Don't include the column if series title is set (it's already formatted) + col: seriesTitle ? null : card._breakoutColumn, + } + : d, + ); - if (clicked) { - // NOTE: certain values such as booleans were coerced to strings at some point. fix them. - parseValues(clicked); - for (const dimension of clicked.dimensions || []) { - parseValues(dimension); - } + dimensions = dimensions.filter( + ({ column }) => + // don't include aggregations since we can't filter on them + column.source !== "aggregation" && + // these columns come from scalar series names + column.source !== "query-transform", + ); - const isLine = this.classList.contains("dot"); - return { - index: isSingleSeriesBar ? -1 : seriesIndex, - element: isLine ? this : null, - event: isLine ? null : d3.event, - ...clicked, - }; + // NOTE: certain values such as booleans were coerced to strings at some point. fix them. + for (const dimension of dimensions) { + dimension.value = parseBooleanStringValue(dimension); } + const column = series[seriesIndex].data.cols[1]; + value = parseBooleanStringValue({ column, value }); + + // We align tooltips differently depending on the type of chart and whether + // the user is hovering/clicked. + // + // On hover, we want to put the tooltip statically next to the hovered element + // *unless* the element is an area. Those are weirdly shaped, so we put the + // tooltip next to the mouse. + // + // On click, it's somewhat reversed. Typically we want the tooltip to appear + // right next to where the user just clicked. The exception is line charts. + // There we want to snap to the closest hovered dot since the voronoi snapping + // we do means the mouse might be slightly off. + const isLine = classList.includes("dot"); + const isArea = classList.includes("area"); + const shouldUseMouseCoordinates = + event.type === "mousemove" ? isArea : !isLine; + + return { + // for single series bar charts, fade the series and highlght the hovered element with CSS + index: isSingleSeriesBar ? -1 : seriesIndex, + element: !shouldUseMouseCoordinates ? element : null, + event: shouldUseMouseCoordinates ? event : null, + data: data.length > 0 ? data : null, + dimensions, + value, + column, + }; } -function parseValues(clicked) { - if (clicked.column && clicked.column.base_type === "type/Boolean") { - if (clicked.value === "true") { - clicked.value = true; - } else if (clicked.value === "false") { - clicked.value = false; +function parseBooleanStringValue({ column, value }) { + if (column && column.base_type === "type/Boolean") { + if (value === "true") { + return true; + } else if (value === "false") { + return false; } } + return value; } -// series = an array of serieses (?) in the chart. There's only one thing in here unless we're dealing with a multiseries chart -function applyChartTooltips( +export function setupTooltips( + { settings, series, isScalarSeries, onHoverChange, onVisualizationClick }, + datas, chart, - series, - isStacked, - isNormalized, - isScalarSeries, - onHoverChange, - onVisualizationClick, + { isBrushing }, ) { - let [ - { - data: { cols }, - }, - ] = series; + const stacked = isStacked(settings, datas); + const normalized = isNormalized(settings, datas); + + const getClickHoverHelper = (target, d) => { + const seriesIndex = determineSeriesIndexFromElement(target, stacked); + const seriesSettings = chart.settings.series(series[seriesIndex]); + const seriesTitle = seriesSettings && seriesSettings.title; + const classList = [...target.classList.values()]; // values returns an iterator, but getClickHoverObject uses Array#includes + + // no tooltips when brushing + if (isBrushing()) { + return null; + } + // no tooltips over lines + if (classList.includes("line")) { + return null; + } + + return getClickHoverObject(d, { + classList, + seriesTitle, + seriesIndex, + series, + isNormalized: normalized, + isScalarSeries, + isStacked: stacked, + event: d3.event, + element: target, + }); + }; + chart.on("renderlet.tooltips", function(chart) { // remove built-in tooltips chart.selectAll("title").remove(); @@ -124,178 +230,18 @@ function applyChartTooltips( if (onHoverChange) { chart .selectAll(".bar, .dot, .area, .line, .bubble") - .on("mousemove", function(d, i) { - // const clicked = clickObjectFromEvent.call(this, d, { - // series, - // isScalarSeries, - // isStacked, - // }); - // onHoverChange(clicked); - - // NOTE: preferably we could just use the above but there's some weird - // edge cases handled by the code below - - const seriesIndex = determineSeriesIndexFromElement(this, isStacked); - const seriesSettings = chart.settings.series(series[seriesIndex]); - const seriesTitle = seriesSettings && seriesSettings.title; - - const card = series[seriesIndex].card; - - const isMultiseries = series.length > 1; - const isBreakoutMultiseries = isMultiseries && card._breakoutColumn; - const isArea = this.classList.contains("area"); - const isBar = this.classList.contains("bar"); - const isSingleSeriesBar = isBar && !isMultiseries; - - // always format the second column as the series name? - function getColumnDisplayName(col) { - // don't replace with series title for breakout multiseries since the series title is shown in the breakout value - if (col === cols[1] && !isBreakoutMultiseries && seriesTitle) { - return seriesTitle; - } else { - return getFriendlyName(col); - } - } - - let data = []; - if (Array.isArray(d.key)) { - // scatter - if (d.key._origin) { - data = d.key._origin.row.map((value, index) => { - const col = d.key._origin.cols[index]; - return { - key: getColumnDisplayName(col), - value: value, - col, - }; - }); - } else { - data = d.key.map((value, index) => ({ - key: getColumnDisplayName(cols[index]), - value: value, - col: cols[index], - })); - } - } else if (d.data) { - // line, area, bar - if (!isSingleSeriesBar) { - cols = series[seriesIndex].data.cols; - } - - data = [ - { - key: getColumnDisplayName(cols[0]), - value: d.data.key, - col: cols[0], - }, - { - key: getColumnDisplayName(cols[1]), - value: isNormalized - ? formatValue(d.data.value, { - number_style: "percent", - column: cols[1], - decimals: cols[1].decimals, - }) - : d.data.value, - col: { ...cols[1] }, - }, - ]; - - // NOTE: The below overcomplicated code is due to using index (i) of - // the element in the DOM, as returned by d3 - // It would be much preferable to somehow get the row more directly - - // now add entries to the tooltip for columns that aren't the X or Y axis. These aren't in - // the normal `cols` array, which is just the cols used in the graph axes; look in `_rawCols` - // for any other columns. If we find them, add them at the end of the `data` array. - // - // To find the actual row where data is coming from is somewhat overcomplicated because i - // seems to follow a strange pattern that doesn't directly correspond to the rows in our - // data. Not sure why but it appears values of i follow this pattern: - // - // seems to follow a strange pattern that doesn't directly correspond to the rows in our - // data. Not sure why but it appears values of i follow this pattern: - // - // [Series 1] i = 7 i = 8 i = 9 i = 10 i = 11 - // [Series 0] i = 1 i = 2 i = 3 i = 4 i = 5 - // [Row 0] [Row 1] [Row 2] [Row 3] [Row 4] - // - // Deriving the rowIndex from i can be done as follows: - // rowIndex = (i % (numRows + 1)) - 1; - // - // example: for series 1, i = 10 - // rowIndex = (10 % 6) - 1 = 4 - 1 = 3 - // - // for series 0, i = 3 - // rowIndex = (3 % 6) - 1 = 3 - 1 = 2 - const seriesData = series[seriesIndex].data || {}; - const rawCols = seriesData._rawCols; - const rows = seriesData && seriesData.rows; - const rowIndex = rows && (i % (rows.length + 1)) - 1; - const row = rowIndex != null && seriesData.rows[rowIndex]; - const rawRow = row && row._origin && row._origin.row; // get the raw query result row - // make sure the row index we've determined with our formula above is correct. Check the - // x/y axis values ("key" & "value") and make sure they match up with the row before setting - // the data for the tooltip - if (rawRow && row[0] === d.data.key && row[1] === d.data.value) { - // rather than just append the additional values we'll just create a new `data` array. - // simply appending the additional values would result in tooltips whose order switches - // between different series. - // Loop over *all* of the columns and create the new array - data = rawCols.map((col, i) => { - // if this was one of the original x/y columns keep the original object because it - // may have the `isNormalized` tweak above. - if (col === data[0].col) { - return data[0]; - } - if (col === data[1].col) { - return data[1]; - } - // otherwise just create a new object for any other columns. - return { - key: getColumnDisplayName(col), - value: rawRow[i], - col: col, - }; - }); - } - } - - if (isBreakoutMultiseries) { - data.unshift({ - key: getFriendlyName(card._breakoutColumn), - // Use series title if it's set - value: seriesTitle ? seriesTitle : card._breakoutValue, - // Don't include the column if series title is set (it's already formatted) - col: seriesTitle ? null : card._breakoutColumn, - }); - } - - data = _.uniq(data, d => d.col); - - onHoverChange({ - // for single series bar charts, fade the series and highlght the hovered element with CSS - index: isSingleSeriesBar ? -1 : seriesIndex, - // for area charts, use the mouse location rather than the DOM element - element: isArea ? null : this, - event: isArea ? d3.event : null, - data: data.length > 0 ? data : null, - }); + .on("mousemove", function(d) { + const hovered = getClickHoverHelper(this, d); + onHoverChange(hovered); }) .on("mouseleave", function() { - if (!onHoverChange) { - return; - } onHoverChange(null); }); } if (onVisualizationClick) { const onClick = function(d) { - const clicked = clickObjectFromEvent.call(this, d, { - series, - isScalarSeries, - }); + const clicked = getClickHoverHelper(this, d); if (clicked) { onVisualizationClick(clicked); } @@ -313,33 +259,3 @@ function applyChartTooltips( } }); } - -export function setupTooltips( - { settings, series, isScalarSeries, onHoverChange, onVisualizationClick }, - datas, - parent, - { isBrushing }, -) { - applyChartTooltips( - parent, - series, - isStacked(settings, datas), - isNormalized(settings, datas), - isScalarSeries, - hovered => { - // disable tooltips while brushing - if (onHoverChange && !isBrushing()) { - // disable tooltips on lines - if ( - hovered && - hovered.element && - hovered.element.classList.contains("line") - ) { - delete hovered.element; - } - onHoverChange(hovered); - } - }, - onVisualizationClick, - ); -} diff --git a/frontend/src/metabase/visualizations/visualizations/Scalar.jsx b/frontend/src/metabase/visualizations/visualizations/Scalar.jsx index 8b128409e18..a731f05d418 100644 --- a/frontend/src/metabase/visualizations/visualizations/Scalar.jsx +++ b/frontend/src/metabase/visualizations/visualizations/Scalar.jsx @@ -82,7 +82,12 @@ export default class Scalar extends Component { }, data: { cols: [ - { base_type: TYPE.Text, display_name: t`Name`, name: "name" }, + { + base_type: TYPE.Text, + display_name: t`Name`, + name: "name", + source: "query-transform", + }, { ...s.data.cols[0] }, ], rows: [[s.card.name, s.data.rows[0][0]]], diff --git a/frontend/test/metabase/visualizations/__support__/visualizations.js b/frontend/test/metabase/visualizations/__support__/visualizations.js index a20fc03a1e6..fecafe0dd3c 100644 --- a/frontend/test/metabase/visualizations/__support__/visualizations.js +++ b/frontend/test/metabase/visualizations/__support__/visualizations.js @@ -26,6 +26,8 @@ export const Column = (col = {}) => ({ display_name: col.display_name || col.name || "column_display_name", }); +export const BooleanColumn = (col = {}) => + Column({ base_type: "type/Boolean", special_type: null, ...col }); export const DateTimeColumn = (col = {}) => Column({ base_type: "type/DateTime", special_type: null, ...col }); export const NumberColumn = (col = {}) => diff --git a/frontend/test/metabase/visualizations/components/LineAreaBarRenderer-bar.unit.spec.js b/frontend/test/metabase/visualizations/components/LineAreaBarRenderer-bar.unit.spec.js index dfa6d4c345b..4cb6581b374 100644 --- a/frontend/test/metabase/visualizations/components/LineAreaBarRenderer-bar.unit.spec.js +++ b/frontend/test/metabase/visualizations/components/LineAreaBarRenderer-bar.unit.spec.js @@ -38,8 +38,16 @@ function MainSeries(chartType, settings = {}, { key = "A", value = 1 } = {}) { }, data: { cols: [ - StringColumn({ display_name: "Category", source: "breakout" }), - NumberColumn({ display_name: "Sum", source: "aggregation" }), + StringColumn({ + display_name: "Category", + source: "breakout", + field_ref: ["field-id", 1], + }), + NumberColumn({ + display_name: "Sum", + source: "aggregation", + field_ref: ["field-id", 2], + }), ], rows: [[key, value]], }, @@ -51,8 +59,16 @@ function ExtraSeries(count = 2) { card: {}, data: { cols: [ - StringColumn({ display_name: "Category", source: "breakout" }), - NumberColumn({ display_name: "Count", source: "aggregation" }), + StringColumn({ + display_name: "Category", + source: "breakout", + field_ref: ["field-id", 3], + }), + NumberColumn({ + display_name: "Count", + source: "aggregation", + field_ref: ["field-id", 4], + }), ], rows: [["A", count]], }, diff --git a/frontend/test/metabase/visualizations/lib/apply_tooltips.unit.spec.js b/frontend/test/metabase/visualizations/lib/apply_tooltips.unit.spec.js new file mode 100644 index 00000000000..13f0ecae960 --- /dev/null +++ b/frontend/test/metabase/visualizations/lib/apply_tooltips.unit.spec.js @@ -0,0 +1,132 @@ +import moment from "moment"; + +import { getClickHoverObject } from "metabase/visualizations/lib/apply_tooltips"; + +import { + getFormattedTooltips, + BooleanColumn, + DateTimeColumn, + StringColumn, + NumberColumn, +} from "../__support__/visualizations"; + +describe("getClickHoverObject", () => { + it("should return data for tooltip", () => { + const d = { data: { key: "foobar", value: 123 } }; + const cols = [StringColumn(), NumberColumn()]; + const rows = [["foobar", 123]]; + const otherArgs = { + series: [{ data: { cols, rows }, card: {} }], + seriesIndex: 0, + classList: [], + event: {}, + }; + + const obj = getClickHoverObject(d, otherArgs); + + expect(getFormattedTooltips(obj)).toEqual(["foobar", "123"]); + }); + + it("should show the correct tooltip for dates", () => { + const d = { + data: { + key: moment("2016-04-01T00:00:00.000Z", "YYYY-MM-DDTHH:mm:ss.SSSSZ"), + value: 123, + }, + }; + const cols = [DateTimeColumn({ unit: "month" }), NumberColumn()]; + const rows = [ + ["2016-03-01T00:00:00.000Z", 1], + ["2016-04-01T00:00:00.000Z", 2], + ["2016-05-01T00:00:00.000Z", 3], + ]; + const otherArgs = { + series: [{ data: { cols, rows }, card: {} }], + seriesIndex: 0, + classList: [], + event: {}, + }; + + const obj = getClickHoverObject(d, otherArgs); + + expect(getFormattedTooltips(obj)).toEqual(["April, 2016", "2"]); + }); + + // This is an ugly test. It's looking at whether we correctly set event and + // element properties on the returned object. Those are used to determine how + // the tooltips are positioned. + it("should return event/element target correctly", () => { + const d = { data: { key: "foobar", value: 123 } }; + const cols = [StringColumn(), NumberColumn()]; + const rows = [["foobar", 123]]; + const otherArgs = { + series: [{ data: { cols, rows }, card: {} }], + seriesIndex: 0, + element: "DOM element", + }; + + for (const [eventType, klass, shouldUseMouseLocation] of [ + ["mousemove", "bar", false], + ["click", "bar", true], + ["mousemove", "dot", false], + ["click", "dot", false], + ["mousemove", "area", true], + ["click", "area", true], + ]) { + const { event, element } = getClickHoverObject(d, { + ...otherArgs, + classList: [klass], + event: { type: eventType }, + }); + if (shouldUseMouseLocation) { + expect(event).toEqual({ type: eventType }); + expect(element).toEqual(null); + } else { + expect(event).toEqual(null); + expect(element).toEqual("DOM element"); + } + } + }); + + it("should exclude aggregation and query-transform columns from dimensions", () => { + const d = { data: { key: "foobar", value: 123 } }; + const cols = [ + StringColumn(), + NumberColumn({ source: "aggregation" }), + StringColumn({ source: "query-transform" }), + ]; + const rows = [["foobar", 123, "barfoo"]]; + const otherArgs = { + series: [{ data: { cols, rows }, card: {} }], + seriesIndex: 0, + classList: [], + event: {}, + }; + + const { data, dimensions } = getClickHoverObject(d, otherArgs); + + expect(data.map(d => d.col)).toEqual(cols); + expect(dimensions.map(d => d.column)).toEqual([cols[0]]); + }); + + it("should parse boolean strings in boolean columns", () => { + const d = { data: { key: "foobar", value: "true" } }; + const cols = [StringColumn(), BooleanColumn()]; + const rows = [["foobar", "true"]]; + const otherArgs = { + series: [{ data: { cols, rows }, card: {} }], + seriesIndex: 0, + classList: [], + event: {}, + seriesTitle: "better name", + }; + + const { + dimensions: [, { value: dimValue }], + value: dValue, + } = getClickHoverObject(d, otherArgs); + + expect(dimValue).toBe(true); + expect(dValue).toBe(true); + }); +}); -- GitLab