feat(big-number): allow fallback to last available value and fix time range for trend lines #403

ktmud · 2020-03-17T04:12:12Z

🏆 Enhancements

The Big Number chart has two problems related to the handling of null values:

Misleading time range: When there is no data at the beginning or ending periods of the filtered time range, the backend returns no data records for those periods, therefore the trend line will only start rendering from the datetime where there is data. This often leads to confusion or misinterpretations, especially then in the dashboards that have multiple trend line charts filtered to the same time range. They could all be showing similar smooth lines, yet representing very different time ranges.
NULL value at the last record: When the last record returns NULL value for the metric, the Big Number text will render as "No data", but the trend line still renders. Some users have requested to fallback displaying the Big Number with the last available dates.

This PR adds option timeRangeFixed to fix the first problem and adds a "fallback to last available value" behavior to address the second problem.

The timeRangedFixed option is disabled by default. When enabled, the empty starting and ending periods will take space on the x-axis, but there will be no connected lines, dots, or tooltips, just as if the metric values are NULL.

In case the last record has a NULL value for the metric, we will go back to the last available value and display it in grayed out style (see screen shots below).

Superset backend needs to be updated to return query.from_dttm and query.to_dttm. Frontend needs to be updated to add two checkbox controls for these options ( apache/superset#9341).

This PR also rewrote the Big Number chart plugin with Typescript.

Screenshot

Before

After

Above chart has a time range between 2001 to 2014, yet only since 2006 it starts to have data. When timeRangeFixed is enabled, it will render from the beginning of the parsed filter time range (2001), instead of when it starts to actually have data (2006).

In Superset

Before

After

I also increased the default font sizes a little bit.

After

Test Plan

Go to storybook and make sure all examples for Big Number chart work.

There is also a new unit test to ensure timeRangeUseFallback works.

Reviewer

cc @kristw @etr2460

In Superset, when a timeseries query has no data at the beginning period or end period of the filtered time range, there will not no data records at those periods, hence the trendline in Big Number chart would not render those periods. This often causes confusion and misinterpretaiton in dashboards, especially for those with multiple trendline charts aligned with each other. They could all be a very smooth line, but actually showing very different time ranges. This PR adds an option "alignTimeRange" to apply the filtered time range on the xAxis. Date periods for empty data will be rendered, but there will be no connected lines, dots, or tooltips for them. It's possible to still show tooltips for those periods, but I decided not to do that as: 1) it makes things much more complicated; 2) I don't want to confuse zero or nulls with empty data.

netlify · 2020-03-17T17:02:40Z

Deploy preview for superset-ui-plugins ready!

Built with commit 3912f58

https://deploy-preview-403--superset-ui-plugins.netlify.com

kristw

There are eslint errors.

/home/travis/build/apache-superset/superset-ui-plugins/packages/superset-ui-legacy-preset-chart-big-number/src/index.js
  1:49  error  Missing file extension "ts" for "./BigNumber/index"       import/extensions
  2:54  error  Missing file extension "ts" for "./BigNumberTotal/index"  import/extensions
/home/travis/build/apache-superset/superset-ui-plugins/packages/superset-ui-legacy-preset-chart-big-number/src/preset.js
  20:34  error  Missing file extension "ts" for "./BigNumber"       import/extensions
  21:39  error  Missing file extension "ts" for "./BigNumberTotal"  import/extensions

Don't add the ts extension like it suggests. Just apply eslint-disable because once these file are transpiled into js in lib and esm, having the .ts extension in import statement will make it fail to find the file.

packages/superset-ui-legacy-preset-chart-big-number/src/BigNumber/BigNumber.tsx

packages/superset-ui-legacy-preset-chart-big-number/package.json

packages/superset-ui-legacy-preset-chart-big-number/src/BigNumber/BigNumber.tsx

codecov · 2020-03-17T19:21:19Z

Codecov Report

Merging #403 into master will increase coverage by 0.55%.
The diff coverage is 27.5%.

@@            Coverage Diff            @@
##           master    #403      +/-   ##
=========================================
+ Coverage    2.83%   3.38%   +0.55%     
=========================================
  Files         186     187       +1     
  Lines        5821    5844      +23     
  Branches      373     405      +32     
=========================================
+ Hits          165     198      +33     
+ Misses       5631    5605      -26     
- Partials       25      41      +16

Impacted Files	Coverage Δ
...reset-chart-big-number/src/BigNumberTotal/index.ts	`0% <ø> (ø)`
...et-ui-legacy-preset-chart-big-number/src/preset.ts	`0% <ø> (ø)`
...acy-preset-chart-big-number/src/BigNumber/index.ts	`0% <ø> (ø)`
...reset-chart-big-number/src/BigNumber/BigNumber.tsx	`0% <0%> (ø)`
...t-chart-big-number/src/BigNumber/transformProps.ts	`57.14% <57.14%> (ø)`
...number/src/utils/getTimeFormatterForGranularity.ts	`83.33% <83.33%> (ø)`

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 c4c3f4a...3912f58. Read the comment docs.

ktmud · 2020-03-17T19:40:59Z

Don't add the ts extension like it suggests. Just apply eslint-disable because once these file are transpiled into js in lib and esm, having the .ts extension in import statement will make it fail to find the file.

I moved all files to ts instead and made Storybook read index.ts as well: https://github.com/apache-superset/superset-ui-plugins/pull/403/files#diff-55dda99e8a651cb4fdb5eb443cb1f8b5R50

kristw · 2020-03-17T20:52:28Z

packages/superset-ui-legacy-preset-chart-big-number/package.json

    "shortid": "^2.2.14"
  },
  "peerDependencies": {
+    "d3-color": "^1.2.3",


Let's keep this one as dependencies.

Any reason why would it be better? I thought it's preferable to have it in peerDependencies as long as it is in the dependencies of superset-frontend.

Usually peerDependencies should be use when only one instance of that lib should exist in the entire app. This could be due to the use of singleton or because the dependency library is very heavy. The downside of peerDependencies is it put more responsibility for the app developers who are consuming the library.

Need to add that library to the app-level dependencies.

If every lib lists everything as peerDependencies, the app will have a long list of dependencies that many of them are not used directly in the app code.

If this lib stop using d3-color, app developers need to remove it from their app's dependencies or it will become redundant dependencies.

If the app needs d3-color version that is not compatible with this library, then there is a conflict.

So if the library is small and does not have singleton requirement, we should list as dependencies and rely on npm or yarn to handle that. This will encapsulate the dependencies and avoid the additional task for app developers to manage d3-color as its dependencies.

Understood. And this totally makes sense. I'm wondering whether we can issue an "official" list of peerDependencies supported by Superset that plugin developers can trust will always be available and stable. I'd imagine d3 packages to be one of them.

"official" list of peerDependencies supported by Superset that plugin developers can trust will always be available and stable. I'd imagine d3 packages to be one of them.

Each plugin may be developed on current version of d3 but then later on if d3 upgrades over time, but some plugins did not migrate, some did, it will cause problem what version Superset should say official, because everybody tries to point to the same d3. At this point, it is not clear what value we actually gain from having d3 as peerDependencies. It is better to use dependencies.

Also, these plugins can also be used in another app that is not Superset.

I'd say packing multi versions of d3 to the client side is still something we should actively try to avoid. Maybe incorporate something like InspectPack would help.

kristw

Agree with the direction of the solution.

packages/superset-ui-legacy-preset-chart-big-number/src/BigNumber/transformProps.ts

packages/superset-ui-legacy-preset-chart-big-number/src/BigNumber/BigNumber.tsx

kristw · 2020-03-27T23:51:19Z

Very nice. Could we move the big number up a bit in case of fallback so they align?
That's last thing and I'll merge.

Keep running into building errors locally, so upgrade nimbus and fix all related packages to the working latest version.

kristw · 2020-03-30T16:33:22Z

package.json

-    "@airbnb/config-prettier": "^2.0.4",
-    "@airbnb/config-typescript": "^2.1.2",
-    "@airbnb/nimbus": "^2.1.3",
+    "@airbnb/config-babel": "2.2.4",


Why pin to specific version without ^?

Was having trouble building. Couldn't figure out why. Just don't want to deal with the unpredictability. Sometimes even with the implied "not a breaking change" in a minor version upgrade, things might still break.

This repo already check in yarn.lock so if you did not modify these package version, it should be the same version that was working in master. Could you try reseting yarn.lock and package.json to master's version while keeping the changes you made to BigNumber's package.json and yarn install again?

Weird, it works... I'm pretty sure I tried that yesterday... Maybe it's because I inadvertently pined @types/shortid.

I think it's probably not a bad idea to pin devDependencies as they are unlikely to cause dependency hell and the added stability is valuable. Renovate actually pin the dependencies for you once you set it up.

packages/superset-ui-legacy-preset-chart-big-number/package.json

… range for trend lines (apache-superset#403) * feat(big-number): add option to align time range In Superset, when a timeseries query has no data at the beginning period or end period of the filtered time range, there will not no data records at those periods, hence the trendline in Big Number chart would not render those periods. This often causes confusion and misinterpretaiton in dashboards, especially for those with multiple trendline charts aligned with each other. They could all be a very smooth line, but actually showing very different time ranges. This PR adds an option "alignTimeRange" to apply the filtered time range on the xAxis. Date periods for empty data will be rendered, but there will be no connected lines, dots, or tooltips for them. It's possible to still show tooltips for those periods, but I decided not to do that as: 1) it makes things much more complicated; 2) I don't want to confuse zero or nulls with empty data. * fix(big-number): disable alignRange by default * refactor(big-number): migrate to Typescript * fix(big-number): typescript build * fix(big-number): change tooltip trigger; fix storybook * fix(big-number): move @types to dependencies * fix(big-number): move all files to ts * build(big-number): add @types/d3-color as dependency * refactor(big-number): remove renderTooltip as prop * feat(big-number): add timeRangeUseFallback options and some refactor * fix(big-number): update formatting functions * fix(big-number): update copy for no data * fix(big-number): address PR feedbacks * feat(big-number): replace timeRangeUseFallback with bigNumberFallback * fix: upgrade @types/react-bootstrap * build(big-number): move react-bootstrap to dependencies * refactor(big-number): more coherent types * feat(big-number): use alert box for fallback values * build(big-number): remove react-bootstrap * build: upgrade nimbus and fix versions Keep running into building errors locally, so upgrade nimbus and fix all related packages to the working latest version. * feat(big-number): adjust fallback warning alignment * build: use a non-fixed version for @types/shortid * build: revert package versions in main

ktmud added 3 commits March 16, 2020 12:02

fix(big-number): disable alignRange by default

3f80d84

refactor(big-number): migrate to Typescript

14e7c48

ktmud requested a review from a team as a code owner March 17, 2020 04:12

ktmud changed the title ~~Big number align range~~ feat(big-number): allow fixed time range Mar 17, 2020

fix(big-number): typescript build

c852f2b

kristw reviewed Mar 17, 2020

View reviewed changes

ktmud added 3 commits March 17, 2020 11:41

fix(big-number): change tooltip trigger; fix storybook

3670f55

fix(big-number): move @types to dependencies

bcf0255

fix(big-number): move all files to ts

af8512e

kristw reviewed Mar 17, 2020

View reviewed changes

ktmud force-pushed the big-number-align-range branch from 829826c to af8512e Compare March 17, 2020 20:54

ktmud added 3 commits March 17, 2020 14:37

build(big-number): add @types/d3-color as dependency

252e0ba

refactor(big-number): remove renderTooltip as prop

69874ff

feat(big-number): add timeRangeUseFallback options and some refactor

b561cf6

ktmud changed the title ~~feat(big-number): allow fixed time range~~ feat(big-number): allow timeRangeUseFallback and timeRangeFixed option Mar 21, 2020

fix(big-number): update formatting functions

f085a86

ktmud mentioned this pull request Mar 21, 2020

feat(visualization): add timeRangeFixed to Big Number w/ Trendlines apache/superset#9341

Merged

7 tasks

fix(big-number): update copy for no data

27e7875

kristw reviewed Mar 23, 2020

View reviewed changes

fix(big-number): address PR feedbacks

e4b3d04

ktmud changed the title ~~feat(big-number): allow timeRangeUseFallback and timeRangeFixed option~~ feat(big-number): allow fallback to last available value and fix time range for trend lines Mar 25, 2020

ktmud added 3 commits March 24, 2020 23:05

feat(big-number): replace timeRangeUseFallback with bigNumberFallback

e6b0643

fix: upgrade @types/react-bootstrap

8cf8224

build(big-number): move react-bootstrap to dependencies

116b0fb

kristw reviewed Mar 25, 2020

View reviewed changes

refactor(big-number): more coherent types

3113b2d

feat(big-number): use alert box for fallback values

cc2c36e

kristw approved these changes Mar 27, 2020

View reviewed changes

ktmud added 3 commits March 27, 2020 19:58

build(big-number): remove react-bootstrap

61db243

build: upgrade nimbus and fix versions

d98f2d2

Keep running into building errors locally, so upgrade nimbus and fix all related packages to the working latest version.

feat(big-number): adjust fallback warning alignment

14da264

ktmud force-pushed the big-number-align-range branch from 522bda3 to 14da264 Compare March 30, 2020 02:56

kristw reviewed Mar 30, 2020

View reviewed changes

ktmud added 2 commits March 30, 2020 10:34

build: use a non-fixed version for @types/shortid

7476ea4

build: revert package versions in main

3912f58

kristw merged commit 60121a0 into apache-superset:master Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(big-number): allow fallback to last available value and fix time range for trend lines #403

feat(big-number): allow fallback to last available value and fix time range for trend lines #403

ktmud commented Mar 17, 2020 •

edited

Loading

netlify bot commented Mar 17, 2020 •

edited

Loading

kristw left a comment

codecov bot commented Mar 17, 2020 •

edited

Loading

ktmud commented Mar 17, 2020

kristw Mar 17, 2020

ktmud Mar 17, 2020

kristw Mar 17, 2020 •

edited

Loading

ktmud Mar 17, 2020

kristw Mar 24, 2020

ktmud Mar 24, 2020

kristw left a comment

kristw commented Mar 27, 2020

kristw Mar 30, 2020

ktmud Mar 30, 2020

kristw Mar 30, 2020

ktmud Mar 30, 2020

feat(big-number): allow fallback to last available value and fix time range for trend lines #403

feat(big-number): allow fallback to last available value and fix time range for trend lines #403

Conversation

ktmud commented Mar 17, 2020 • edited Loading

Screenshot

Before

After

In Superset

Before

After

After

Test Plan

Related

Reviewer

netlify bot commented Mar 17, 2020 • edited Loading

kristw left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 17, 2020 • edited Loading

Codecov Report

ktmud commented Mar 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw left a comment

Choose a reason for hiding this comment

kristw commented Mar 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud commented Mar 17, 2020 •

edited

Loading

netlify bot commented Mar 17, 2020 •

edited

Loading

codecov bot commented Mar 17, 2020 •

edited

Loading

kristw Mar 17, 2020 •

edited

Loading