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

[corbinrobb]

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:

1. Change the TextControl to be able to have a min-max value for numbers
2. Set up a control for InputNumber
3. Use SliderControl but have state update onAfterChange
4. 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

[lyndsiWilliams]

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.

[corbinrobb]

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

[villebro]

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]

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.

[codecov]

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]

/testenv up

[github-actions]

@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!

[villebro]

LGTM, thanks for the awesome work here!

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.