feat(timeseries-chart): add percentage threshold input control #17758

corbinrobb · 2021-12-15T05:09:21Z

SUMMARY

Implementation of the feature request in issue #17281

The time-series chart controls have the option to set show values SHOW VALUE and stack the series STACK SERIES. When both are checked there is also the option to show only the series totals ONLY TOTAL but if you want to see all of the values in the stacks you can uncheck ONLY TOTAL.

When ONLY TOTAL is unchecked and the data that's being displayed have small or similar values, the chart labels can become cluttered and tend to sit on top of each other making them difficult to read.

The enhancement is to add an input where you can enter a minimum percentage threshold that would only display labels for values at or above the threshold.

There is an input just like this over in Pie Chart controls as mentioned and shown in the issue that is referenced at the top of this PR.

SHORT DESCRIPTION

Added a TextControl Input for the percentage threshold to the Time-series echarts controls
Added logic in transformProps to conditionally render the labels if above that threshold
Added props to the Time-series Chart storybook story to show these changes
Added some tests fortransformProps for the showValue, onlyTotal, and percentageThreshold props

DETAILED DESCRIPTION

Control Config:

There was already a configuration called showValueSection that holds the other controls mentioned so I added the new control to that.

I set the visibility to only be true if show_value is true, stack is true, and only_total is false. I also set the default to be 5% matching the control in the Pie Chart plugin because it seemed reasonable.

const percentageThresholdControl = {
  name: 'percentage_threshold',
  config: {
    type: 'TextControl',
    label: t('Percentage threshold'),
    renderTrigger: true,
    isFloat: true,
    default: 5,
    description: t(
      'Minimum threshold in percentage points for showing labels.',
    ),
    visibility: ({ controls }: ControlPanelsContainerProps) =>
      Boolean(controls?.show_value?.value) &&
      Boolean(controls?.stack?.value) &&
      Boolean(!controls?.only_total?.value),
  },
};

The showValueSection controls are used in these Time Series Charts.

Area - (Time-series Area Chart)
Bar - (Time-series Bar Chart v2)
Regular - (Time-series Chart)
Line - (Time-series Line Chart)
Stepped Line - (Time-series Stepped Line)
Smooth Line - (Time-series Smooth Line)

All of these charts appear to benefit from this and they all share the same transformProps which is where the logic for these controls exist.

The logic:

We grab the prop that is passed through transformProps by adding it to the list of variables deconstructed out of the formData prop.

Create an array matching the totalStackedValues array but with the calculated threshold value instead of the total.

    totalStackedValues.push(values);
    thresholdValues.push((percentageThreshold / 100) * values);

Pass that down to transformSeries when it is gets called on all the series. Then within that function go on down to where the onlyTotal prop is being checked

BEFORE:

        if (!stack || !onlyTotal || isSelectedLegend) {
            return formatter(numericValue);
        }
        if (seriesIndex === showValueIndexes[dataIndex]) {
          return formatter(totalStackedValues[dataIndex]);
        }
        return '';

Breaking it down super simply, this

returns the series values when NOT stacked or NOT set to show only totals
or
returns the series total when necessary
or
returns empty string

With our thresholdValues we can take separate out !onlyTotal. Then check if the value is greater than or equal to the matching threshold value and return the value or an empty string accordingly.

AFTER:

        if (!stack || isSelectedLegend) return formatter(numericValue);
        if (!onlyTotal) {
          if (numericValue >= thresholdValues[dataIndex]) {
            return formatter(numericValue);
          }
          return '';
        }
        if (seriesIndex === showValueIndexes[dataIndex]) {
          return formatter(totalStackedValues[dataIndex]);
        }
        return '';

Test cases

Added tests for transformProps to test whether props are being successfully transformed. I couldn't find tests for the other showValue controls so I added a couple for them because I was in the area.

Added test cases for:

'should show labels when showValue is true'
'should not show labels when showValue is false'
'should show only totals when onlyTotal is true'
'should show labels on values >= percentageThreshold if onlyTotal is false'
'should not apply percentage threshold when showValue is true and stack is false'

Another thing with the tests is that there were a few different TypeScript errors already in Timeseries/transformProps.test.ts so I did what I could to get them to calm down.

Moving Forward

I wanted to share more thoughts that I had on this feature but feel free to skip over this because it is just ideas.

The TextControl, which is an Antd Input component, works perfectly well but I would say that ideally, we wouldn't want to use a plain input for this control. A percentage can only be between 0 and 100 and the input allows the opportunity to put any numbers into but there isn't a built-in way to add a min and max.

We could add this to the TextControl but I am not the biggest fan of doing this because Antd has the InputNumber component that does have the ability to set a min and max number value. While InputNumber is used in some other places in other controls like with the Y axis bounds, it doesn't appear to have its own control component like the plain Input does with TextControl. I could be wrong about this and if I am please let me know because I would love to try and utilize it for this.

Antd InputNumber link

I also did a version of this with the SliderControl because it felt like it could be a good choice because it has a set range. I ended up deciding against suggesting it for now because it updates the state on onChange and that will update on every 'step' in between the starting number and where it ends. Those state updates rerender the chart and it happens a lot it which makes an unpleasantly sluggish experience.

I believe we could easily get around this by using the prop onAfterChange which is present on Antd slide. The component is currently only using onChange but it wouldn't be too hard to have it take the function passed to onChange and move it to onAfterChange if needed. We could do this by adding an optional boolean value called something like updateAfterChange and if that is true then change the function to run after the change. I haven't tested this so it might not be quite that simple.

I should probably note that onAfterChange runs after onmousup fires. So it would run when the user is done dragging the slider to the desired value and not for every value in between.

Antd Slider link

There is also another option that is shown in the examples on the Slider page where it uses both Slider and InputNumber at the same time while sharing the same state and min/maxes. I liked this one the best because it has both an input and a slider at the same time.

This would require a new control to be made that would combine the two as shown in the Antd Slider examples. I am not sure if this would be useful outside of the Pie Chart and the Time-series charts so it might not be worth making.

Final thoughts for percentage threshold control are:

Change the TextControl to be able to have a min-max value for numbers
Set up a control for InputNumber
Use SliderControl but have state update onAfterChange
Set up a conjoined Slider InputNumber control and use that

These are just ideas though and I am probably wrong about the way to implement them but I figured it wouldn't hurt to share.

BEFORE - BAR

AFTER - BAR

BEFORE - LINE

AFTER - LINE

Doesn't show when stacked is false

Doesn't show when stacked is true and only total is true

Shows when stacked is true and only total is false

---

TESTING INSTRUCTIONS

Open Superset and navigate to Charts
Open up a chart that once stacked would have tightly packed and similar values. I found that Rise & Fall of Video Game Consoles - Area Chart is a good starting place for this
Change the Visualization Type to be one of these: (I suggest Time-series Chart because you can switch between all)
- Time-series Area Chart
- Time-series Bar Chart v2
- Time-series Chart
- Time-series Line Chart
- Time-series Stepped Line
- Time-series Smooth Line
Make sure you have the groupby value set to something with a lot of values. If using Rise & Fall of Video Game Consoles you can leave it on platform or change it to publisher if you are feeling extra spicy
Click the Customize tab at the top and scroll to Chart Options
If you are using Time-series Chart I would suggest changing the Series Style to Bar
Check SHOW VALUE and STACK SERIES, then uncheck ONLY TOTAL
If all goes well you should see the PERCENTAGE THRESHOLD input appear with a value of 5

I would turn on DATA ZOOM too so you can get a better look at the values.

Other than that check it out however you see fit. Maybe check out all the charts mentioned above.

I found it most useful with the bar chart but it seems to be useful on all of them

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

…ries labels

superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts

lyndsiWilliams · 2021-12-17T13:00:04Z

superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts

@@ -49,8 +53,8 @@ describe('EchartsTimeseries tranformProps', () => {
  };

  it('should tranform chart props for viz', () => {
-    const chartProps = new ChartProps(chartPropsConfig);
-    expect(transformProps(chartProps)).toEqual(
+    const chartProps: unknown = new ChartProps(chartPropsConfig);


There are a few prop types described in this component, would any of those work here instead of unknown? I see unknown in other spots in your PR and I'm assuming it's because the props change type, but I'm wondering if you might be able to make it work with a thisTypeOfProps | thatTypeOfProps type of description.

I am actually not sure what is going on with these. I set it to unknown because TS said I had to before it would let me do the type assertion here chartProps as EchartsTimeseriesChartProps. It throws an error when I try to set the type of chartProps to EchartsTimeseriesChartProps when it is defined because their formData prop types are incompatible. This is all strange because they don't actually appear to be incompatible because EchartsTimeseriesChartProps is an extension of ChartProps and ChartProps formData prop is set to be an ambiguous object.

This all appears to be done intentionally and I am not sure what I should do to get rid of the errors because I haven't found a similar test file where this isn't causing TS errors. Give me a little time to look into this more because I am not sure what to do at the moment

This specific line should be something like

const chartProps = new ChartProps<EchartsTimeseriesChartProps>(chartPropsConfig);

However, it seems these types are slightly tangled up right now, and should probably be addressed in a follow-up PR, as I believe it's going to require some more type fixing. I'd also want to enable proper type checking for the test folders, but that also requires a separate PR.

villebro

This is really nice work @corbinrobb ! Tested to work well, and the code looks really solid, along with very nice tests! One minor nit, but apart from that this looks good to go 👍

villebro · 2021-12-20T08:00:36Z

superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts

@@ -49,8 +53,8 @@ describe('EchartsTimeseries tranformProps', () => {
  };

  it('should tranform chart props for viz', () => {
-    const chartProps = new ChartProps(chartPropsConfig);
-    expect(transformProps(chartProps)).toEqual(
+    const chartProps: unknown = new ChartProps(chartPropsConfig);


This specific line should be something like

const chartProps = new ChartProps<EchartsTimeseriesChartProps>(chartPropsConfig);

However, it seems these types are slightly tangled up right now, and should probably be addressed in a follow-up PR, as I believe it's going to require some more type fixing. I'd also want to enable proper type checking for the test folders, but that also requires a separate PR.

superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts

…e, and other minor changes

codecov · 2021-12-21T21:21:27Z

Codecov Report

Merging #17758 (814bff0) into master (63d9693) will decrease coverage by 1.44%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master   #17758      +/-   ##
==========================================
- Coverage   67.77%   66.32%   -1.45%     
==========================================
  Files        1602     1568      -34     
  Lines       64151    61455    -2696     
  Branches     6773     6237     -536     
==========================================
- Hits        43476    40759    -2717     
- Misses      18822    19099     +277     
+ Partials     1853     1597     -256

Flag	Coverage Δ
javascript	`50.92% <80.00%> (-3.92%)`	⬇️

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

Impacted Files	Coverage Δ
...ugins/plugin-chart-echarts/src/Timeseries/types.ts	`100.00% <ø> (ø)`
...tend/plugins/plugin-chart-echarts/src/controls.tsx	`71.42% <50.00%> (-2.26%)`	⬇️
...lugin-chart-echarts/src/Timeseries/transformers.ts	`52.50% <83.33%> (+10.25%)`	⬆️
...gin-chart-echarts/src/Timeseries/transformProps.ts	`60.97% <100.00%> (+7.22%)`	⬆️
...ntend/src/dashboard/util/replaceUndefinedByNull.ts	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/components/Slider/index.tsx	`0.00% <0.00%> (-75.00%)`	⬇️
.../src/explore/components/controls/HiddenControl.tsx	`0.00% <0.00%> (-75.00%)`	⬇️
...t-frontend/src/filters/components/GroupBy/index.ts	`0.00% <0.00%> (-66.67%)`	⬇️
...ntend/src/filters/components/GroupBy/buildQuery.ts	`0.00% <0.00%> (-66.67%)`	⬇️
...end/src/filters/components/TimeGrain/buildQuery.ts	`0.00% <0.00%> (-66.67%)`	⬇️
... and 550 more

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 63d9693...814bff0. Read the comment docs.

lyndsiWilliams · 2022-01-05T17:58:29Z

/testenv up

github-actions · 2022-01-05T18:00:26Z

@lyndsiWilliams Ephemeral environment spinning up at http://34.221.152.42:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

lyndsiWilliams

Just checked this out on the test environment and it runs just as described - great testing instructions by the way! Since we'll be moving the TS technicalities to another PR, this looks great! Nice job on this! ✨

betodealmeida

This look great, amazing job!

villebro

One last comment, other than that LGTM!

superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx

villebro

LGTM, thanks for the awesome work here!

github-actions · 2022-01-12T20:46:49Z

Ephemeral environment shutdown and build artifacts deleted.

…e#17758) * feat(timeseries-chart): add percentage threshold control for stack series labels * feat: move threshold vlues to an array * add tests for showValue, onlyTotal, and percentThreshold * feat: add another test * revert ChartProps typesetting, fix misnamed variable on form data type, and other minor changes * fix percentage threshold push equation * fix percentage threshold push equation in tests * change default on control to match form * attempt fix form defaults import Co-authored-by: Corbin Robb <corbin@Corbins-MacBook-Pro.local>

Corbin Robb added 2 commits December 14, 2021 17:04

feat(timeseries-chart): add percentage threshold control for stack se…

c281cbe

…ries labels

feat: move threshold vlues to an array

21d04d6

pull-request-size bot added the size/M label Dec 15, 2021

add tests for showValue, onlyTotal, and percentThreshold

03a668b

pull-request-size bot added size/L and removed size/M labels Dec 16, 2021

feat: add another test

6f89272

AAfghahi reviewed Dec 16, 2021

View reviewed changes

superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts Show resolved Hide resolved

lyndsiWilliams reviewed Dec 17, 2021

View reviewed changes

villebro reviewed Dec 20, 2021

View reviewed changes

revert ChartProps typesetting, fix misnamed variable on form data typ…

1b3880f

…e, and other minor changes

corbinrobb changed the title ~~feat(timeseries-chart): [WIP] add percentage threshold input control~~ feat(timeseries-chart): add percentage threshold input control Dec 20, 2021

corbinrobb marked this pull request as ready for review December 20, 2021 15:41

Corbin Robb added 2 commits December 20, 2021 09:28

fix percentage threshold push equation

dfc2b36

fix percentage threshold push equation in tests

5bc821f

eschutho added the Superset-Community-Partners Preset community partner program participants label Jan 5, 2022

lyndsiWilliams approved these changes Jan 6, 2022

View reviewed changes

corbinrobb requested a review from villebro January 6, 2022 16:28

betodealmeida approved these changes Jan 10, 2022

View reviewed changes

villebro reviewed Jan 11, 2022

View reviewed changes

superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx Outdated Show resolved Hide resolved

Corbin Robb added 2 commits January 11, 2022 13:20

change default on control to match form

4f1f03f

attempt fix form defaults import

814bff0

villebro approved these changes Jan 12, 2022

View reviewed changes

lyndsiWilliams merged commit 6bd4dd2 into apache:master Jan 12, 2022

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(timeseries-chart): add percentage threshold input control #17758

feat(timeseries-chart): add percentage threshold input control #17758

corbinrobb commented Dec 15, 2021 •

edited

lyndsiWilliams Dec 17, 2021 •

edited

corbinrobb Dec 17, 2021

villebro Dec 20, 2021

villebro left a comment

villebro Dec 20, 2021

codecov bot commented Dec 21, 2021 •

edited

lyndsiWilliams commented Jan 5, 2022

github-actions bot commented Jan 5, 2022

lyndsiWilliams left a comment

betodealmeida left a comment

villebro left a comment

villebro left a comment

github-actions bot commented Jan 12, 2022

feat(timeseries-chart): add percentage threshold input control #17758

feat(timeseries-chart): add percentage threshold input control #17758

Conversation

corbinrobb commented Dec 15, 2021 • edited

SUMMARY

SHORT DESCRIPTION

DETAILED DESCRIPTION

Control Config:

The logic:

Test cases

Moving Forward

BEFORE - BAR

AFTER - BAR

BEFORE - LINE

AFTER - LINE

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

lyndsiWilliams Dec 17, 2021 • edited

Choose a reason for hiding this comment

corbinrobb Dec 17, 2021

Choose a reason for hiding this comment

villebro Dec 20, 2021

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

villebro Dec 20, 2021

Choose a reason for hiding this comment

codecov bot commented Dec 21, 2021 • edited

Codecov Report

lyndsiWilliams commented Jan 5, 2022

github-actions bot commented Jan 5, 2022

lyndsiWilliams left a comment

Choose a reason for hiding this comment

betodealmeida left a comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 12, 2022

corbinrobb commented Dec 15, 2021 •

edited

lyndsiWilliams Dec 17, 2021 •

edited

codecov bot commented Dec 21, 2021 •

edited