feat(explore): Make metric title respond to changes immediately #12747

kgabryje · 2021-01-25T20:55:50Z

SUMMARY

When changes are made in metrics popover, the title should show current value immediately rather than after saving and reopening the popover. When there is no custom title set - display title based on current metric value. If there is a custom title set - always display that title, ignore changes in metric values.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Nagranie.z.ekranu.2021-01-25.o.21.46.58.mov

TEST PLAN

ADDITIONAL INFORMATION

Has associated issue: closes [explore]make popover title change responsive #12608
Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

CC @villebro @junlincc @adam-stasiak @ktmud

codecov-io · 2021-01-25T21:40:09Z

Codecov Report

Merging #12747 (72a5e44) into master (044d1ae) will decrease coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #12747      +/-   ##
==========================================
- Coverage   67.00%   66.81%   -0.20%     
==========================================
  Files        1022     1022              
  Lines       50047    50071      +24     
  Branches     4914     4930      +16     
==========================================
- Hits        33533    33453      -80     
- Misses      16390    16494     +104     
  Partials      124      124

Flag	Coverage Δ
cypress	`50.93% <88.57%> (+0.04%)`	⬆️
javascript	`61.72% <68.57%> (+0.02%)`	⬆️
python	`63.74% <ø> (-0.35%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../controls/MetricControl/AdhocMetricEditPopover.jsx	`80.62% <100.00%> (+0.46%)`	⬆️
...ntrols/MetricControl/AdhocMetricPopoverTrigger.tsx	`94.73% <100.00%> (+3.07%)`	⬆️
superset/db_engines/hive.py	`0.00% <0.00%> (-85.72%)`	⬇️
superset/db_engine_specs/hive.py	`73.84% <0.00%> (-17.31%)`	⬇️
superset/db_engine_specs/presto.py	`81.38% <0.00%> (-6.71%)`	⬇️
superset/views/database/mixins.py	`80.70% <0.00%> (-1.76%)`	⬇️
superset/models/core.py	`88.04% <0.00%> (-0.82%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 044d1ae...72a5e44. Read the comment docs.

villebro

A few comments

villebro · 2021-01-26T07:39:18Z

superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover.jsx

+  componentDidUpdate(prevProps, prevState) {
+    if (
+      prevState.adhocMetric?.label !== this.state.adhocMetric?.label ||
+      prevState.savedMetric?.metric_name !== this.state.savedMetric?.metric_name


do we need to add a prevState.savedMetric?.verbose_name !== this.state.savedMetric?.verbose_name here?

I don't think so - metric_name check is used only to determine whether a different saved metric has been chosen. metric_name is unique, so it's sufficient to compare only this field.

villebro · 2021-01-26T07:47:07Z

superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx

+    newMetric: AdhocMetric | Record<string, any>,
+    oldMetric: AdhocMetric | Record<string, any>,


Is Record<string, any> for saved metrics here? Could this be savedMetricType? (Unrelated bycatch, this type should probably be called SavedMetricType)

Maybe import { Metric } from '@superset-ui/core'; ?

@ktmud Good idea! 👍

adam-stasiak · 2021-01-26T20:23:47Z

I guess name used once should not be cached when metric was modified.
In my scenario
I set Simple metric and set my name. Then move to SAVED value and back to SIMPLE. I would expect to see name reflecting SIMPLE parameters
https://user-images.githubusercontent.com/25153919/105900651-a21b9700-601c-11eb-87df-6d6f99b67d0a.mov

adam-stasiak · 2021-01-26T20:38:21Z

My second concern is whether SAVED METRICS Value chosen should reset custom name and set valid for this SAVED METRIC.
After Save I can see SAVED METRIC value is used as name for this metric.

Nagranie.z.ekranu.2021-01-26.o.21.35.28.mov

junlincc · 2021-01-27T06:12:28Z

I guess name used once should not be cached when metric was modified.

thank you @adam-stasiak, yes this seems to be an issue. we should address it in this same PR.

junlincc · 2021-01-27T06:14:22Z

My second concern is whether SAVED METRICS Value chosen should reset custom name and set valid for this SAVED METRIC.
After Save I can see SAVED METRIC value is used as name for this metric.

this one as well.
BUT, if addressing both @adam-stasiak 's concerns take more than 1 story point, we probably should let them go. @kgabryje

adam-stasiak · 2021-01-27T17:25:24Z

Tested after changes 🟢

kgabryje · 2021-01-27T17:26:41Z

Thanks @adam-stasiak 🙂
CC: @junlincc @villebro

* Make metric title respond to changes immediately * Bug fix * Change type to Metric * Bug fix

superset-github-bot bot added the Polidea label Jan 25, 2021

pull-request-size bot added the size/M label Jan 25, 2021

junlincc added the explore:metrics Related to metrics of Explore label Jan 25, 2021

junlincc requested review from junlincc, villebro and rusackas January 25, 2021 21:26

villebro reviewed Jan 26, 2021

View reviewed changes

villebro approved these changes Jan 26, 2021

View reviewed changes

kgabryje added 4 commits January 27, 2021 16:35

Make metric title respond to changes immediately

a433fb9

Bug fix

151b76a

Change type to Metric

6d0755d

Bug fix

72a5e44

kgabryje force-pushed the feat/metric-title branch from bd98427 to 72a5e44 Compare January 27, 2021 16:35

pull-request-size bot added size/L and removed size/M labels Jan 27, 2021

villebro merged commit 14de7b4 into apache:master Jan 28, 2021

junlincc added the v1.0.1 label Jan 28, 2021

villebro pushed a commit that referenced this pull request Jan 29, 2021

feat(explore): Make metric title respond to changes immediately (#12747)

2fc03f8

* Make metric title respond to changes immediately * Bug fix * Change type to Metric * Bug fix

mistercrunch added 🍒 1.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(explore): Make metric title respond to changes immediately #12747

feat(explore): Make metric title respond to changes immediately #12747

kgabryje commented Jan 25, 2021

codecov-io commented Jan 25, 2021 •

edited

Loading

villebro left a comment

villebro Jan 26, 2021

kgabryje Jan 26, 2021

villebro Jan 26, 2021

ktmud Jan 26, 2021

ktmud Jan 26, 2021

kgabryje Jan 26, 2021 •

edited

Loading

adam-stasiak commented Jan 26, 2021 •

edited

Loading

adam-stasiak commented Jan 26, 2021

junlincc commented Jan 27, 2021

junlincc commented Jan 27, 2021

adam-stasiak commented Jan 27, 2021

kgabryje commented Jan 27, 2021

		newMetric: AdhocMetric \| Record<string, any>,
		oldMetric: AdhocMetric \| Record<string, any>,

feat(explore): Make metric title respond to changes immediately #12747

feat(explore): Make metric title respond to changes immediately #12747

Conversation

kgabryje commented Jan 25, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov-io commented Jan 25, 2021 • edited Loading

Codecov Report

villebro left a comment

Choose a reason for hiding this comment

villebro Jan 26, 2021

Choose a reason for hiding this comment

kgabryje Jan 26, 2021

Choose a reason for hiding this comment

villebro Jan 26, 2021

Choose a reason for hiding this comment

ktmud Jan 26, 2021

Choose a reason for hiding this comment

ktmud Jan 26, 2021

Choose a reason for hiding this comment

kgabryje Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

adam-stasiak commented Jan 26, 2021 • edited Loading

adam-stasiak commented Jan 26, 2021

junlincc commented Jan 27, 2021

junlincc commented Jan 27, 2021

adam-stasiak commented Jan 27, 2021

kgabryje commented Jan 27, 2021

codecov-io commented Jan 25, 2021 •

edited

Loading

kgabryje Jan 26, 2021 •

edited

Loading

adam-stasiak commented Jan 26, 2021 •

edited

Loading