From 55f36d7c093092dbd4c5456c04ddddd8a669b319 Mon Sep 17 00:00:00 2001
From: adam-james <21064735+adam-james-v@users.noreply.github.com>
Date: Thu, 1 Sep 2022 14:21:30 -0700
Subject: [PATCH] Send show_values key to static-viz js (#25116)
* Send show_values key to static-viz js
The static viz components expect certain keys on the settings map passed into the js. At this time, the contract
between the backend (Clojure) and the static-viz (js) feels a little under-specified. However, for the change to XY
Charts here, we follow the existing method in code of checking the backend viz-settings map for some key, and passing
it into js in a 'simplified' form. For instance, in this change, `:graph.show-values` is taken and placed into the
settings map as `:show_values` for the static-viz js to use.
* De-dupe key
* Keep this PR super simple, don't eliminate sparkline stuff yet
* Add :line earlier in cond, try to fix tests that fail
* Get the :line conditional in the right place, allowing :scalar
---
src/metabase/pulse/render.clj | 10 ++++------
src/metabase/pulse/render/body.clj | 23 ++++++++++++-----------
src/metabase/util/ui_logic.clj | 4 ++--
test/metabase/pulse/render_test.clj | 8 ++++----
4 files changed, 22 insertions(+), 23 deletions(-)
diff --git a/src/metabase/pulse/render.clj b/src/metabase/pulse/render.clj
index 8d577fc7ad7..60e0909743e 100644
--- a/src/metabase/pulse/render.clj
+++ b/src/metabase/pulse/render.clj
@@ -94,6 +94,7 @@
:waterfall} display-type)
(chart-type display-type "display-type is %s" display-type)
+ ;; for scalar/smartscalar, the display-type might actually be :line, so we can't have line above
(= @col-sample-count @row-sample-count 1)
(chart-type :scalar "result has one row and one column")
@@ -107,16 +108,13 @@
(not (#{:combo} display-type)))
(chart-type :multiple "result has multiple card semantics, a multiple chart")
- ;; Default behavior of these to be sparkline, unless the columns and rows don't behave and display type is correct,
- ;; upon which they're lines
+ ;; we have to check when display-type is :line that there are enough rows/cols to actually create a line chart
+ ;; if there is only 1 row and 1 col, the chart should be considered scalar, actually.
(and (= @col-sample-count 2)
(> @row-sample-count 1)
(number-field? @col-2)
(not (#{:waterfall :pie :table :area} display-type)))
- (chart-type :sparkline "result has 2 cols (%s and %s (number)) and > 1 row" (col-description @col-1) (col-description @col-2))
-
- (= display-type :line)
- (chart-type display-type "display-type is %s" display-type)
+ (chart-type :line "result has 2 cols (%s and %s (number)) and > 1 row" (col-description @col-1) (col-description @col-2))
(and (= @col-sample-count 2)
(number-field? @col-2)
diff --git a/src/metabase/pulse/render/body.clj b/src/metabase/pulse/render/body.clj
index 97333806e86..7445467300b 100644
--- a/src/metabase/pulse/render/body.clj
+++ b/src/metabase/pulse/render/body.clj
@@ -303,8 +303,8 @@
For further details look at frontend/src/metabase/static-viz/XYChart/types.ts"
[x-col y-col labels {::mb.viz/keys [column-settings] :as viz-settings}]
- (let [default-format {:number_style "decimal"
- :currency "USD"
+ (let [default-format {:number_style "decimal"
+ :currency "USD"
:currency_style "symbol"}
x-col-settings (or (settings-from-column x-col column-settings) {})
y-col-settings (or (settings-from-column y-col column-settings) {})
@@ -319,13 +319,14 @@
default-x-type (if (isa? (:effective_type x-col) :type/Temporal)
"timeseries"
"ordinal")]
- {:colors (public-settings/application-colors)
- :stacking (if (:stackable.stack_type viz-settings) "stack" "none")
- :x {:type (or (:graph.x_axis.scale viz-settings) default-x-type)
- :format x-format}
- :y {:type (or (:graph.y_axis.scale viz-settings) "linear")
- :format y-format}
- :labels labels}))
+ {:colors (public-settings/application-colors)
+ :stacking (if (:stackable.stack_type viz-settings) "stack" "none")
+ :show_values (boolean (:graph.show_values viz-settings))
+ :x {:type (or (:graph.x_axis.scale viz-settings) default-x-type)
+ :format x-format}
+ :y {:type (or (:graph.y_axis.scale viz-settings) "linear")
+ :format y-format}
+ :labels labels}))
(defn- set-default-stacked
"Default stack type is stacked for area chart with more than one metric.
@@ -615,8 +616,8 @@
Use the combo charts for every chart-type in line area bar because we get multiple chart series for cheaper this way."
[chart-type render-type _timezone-id card dashcard {:keys [cols rows viz-settings] :as data}]
(let [viz-settings (merge viz-settings (:visualization_settings dashcard))
- x-axis-rowfn (ui-logic/mult-x-axis-rowfn card data)
- y-axis-rowfn (ui-logic/mult-y-axis-rowfn card data)
+ x-axis-rowfn (or (ui-logic/mult-x-axis-rowfn card data) #(vector (first %)))
+ y-axis-rowfn (or (ui-logic/mult-y-axis-rowfn card data) #(vector (second %)))
x-rows (filter some? (map x-axis-rowfn rows))
y-rows (filter some? (map y-axis-rowfn rows))
joined-rows (mapv vector x-rows y-rows)
diff --git a/src/metabase/util/ui_logic.clj b/src/metabase/util/ui_logic.clj
index 309e610b36a..ce815f3773d 100644
--- a/src/metabase/util/ui_logic.clj
+++ b/src/metabase/util/ui_logic.clj
@@ -90,7 +90,7 @@
(let [metrics (some-> card
(get-in [:visualization_settings :graph.metrics]))
col-indices (map #(column-name->index % results) metrics)]
- (when (seq? col-indices)
+ (when (seq col-indices)
(fn [row]
(let [res (vec (for [idx col-indices]
(get row idx)))]
@@ -105,7 +105,7 @@
(let [dimensions (some-> card
(get-in [:visualization_settings :graph.dimensions]))
col-indices (map #(column-name->index % results) dimensions)]
- (when (seq? col-indices)
+ (when (seq col-indices)
(fn [row]
(let [res (vec (for [idx col-indices]
(get row idx)))]
diff --git a/test/metabase/pulse/render_test.clj b/test/metabase/pulse/render_test.clj
index 66930e667d8..f38cb8b06b9 100644
--- a/test/metabase/pulse/render_test.clj
+++ b/test/metabase/pulse/render_test.clj
@@ -23,12 +23,12 @@
(render-pulse-card card)))
(deftest render-test
- (testing "if the pulse rendered correctly it will have this one row that says \"November 2015\" (not sure why)"
+ (testing "if the pulse rendered correctly it will have an img tag."
(is (some? (mbql.u/match-one (render-results
(mt/mbql-query checkins
{:aggregation [[:count]]
:breakout [!month.date]}))
- [:td _ "November, 2015"])))))
+ [:img _])))))
(deftest render-error-test
(testing "gives us a proper error if we have erroring card"
@@ -91,7 +91,7 @@
:rows [["A" 2]]})))
;; timeseries line chart
- (is (= :sparkline
+ (is (= :line
(render/detect-pulse-chart-type {:display :line}
{}
{:cols [{:base_type :type/Temporal}
@@ -99,7 +99,7 @@
:rows [[#t "2020" 2]
[#t "2021" 3]]})))
;; Category line chart
- (is (= :sparkline
+ (is (= :line
(render/detect-pulse-chart-type {:display :line}
{}
{:cols [{:base_type :type/Text}
--
GitLab