superset icon indicating copy to clipboard operation
superset copied to clipboard

feat: [WIP] support x-filter on echarts timeseries zoom

Open fullergalway opened this issue 3 years ago • 6 comments

SUMMARY

It would be nice to set a time range filter after using the zoom tool

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

output

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • [ ] Has associated issue:
  • [ ] Required feature flags:
  • [X] 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
  • [X] Introduces new feature or API
  • [ ] Removes existing feature or API

A couple of issues arise:

  1. How to get setDataMask to also update this chart, when doing so is not the default option (done in this example after edit dashboard, change properties so filter is applied to this chart too). Problem otherwise is that the zoom is lost when the filters apply.
  2. A way to create and/or update a corresponding native filter would be nice.

fullergalway avatar Nov 21 '22 15:11 fullergalway

Codecov Report

Merging #22183 (fef70d7) into master (f64423a) will decrease coverage by 0.00%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #22183      +/-   ##
==========================================
- Coverage   66.87%   66.86%   -0.01%     
==========================================
  Files        1847     1847              
  Lines       70561    70571      +10     
  Branches     7739     7741       +2     
==========================================
  Hits        47186    47186              
- Misses      21374    21383       +9     
- Partials     2001     2002       +1     
Flag Coverage Δ
javascript 53.82% <0.00%> (-0.02%) :arrow_down:

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

Impacted Files Coverage Δ
...gin-chart-echarts/src/Timeseries/transformProps.ts 46.93% <0.00%> (-5.34%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov[bot] avatar Nov 22 '22 08:11 codecov[bot]

Thanks for the contribution @fullergalway! This is an intriguing feature addition! The designers involved with Superset are rethinking a bit of iconography and so forth around the crossfilter experience, so @kasiazjc might have some input on the design implications. Doubly so, since this creates a new pattern of manually emitting a filter.

We've usually steered users toward dashboard filters for this sort of time range filter, but I think what you've done here makes sense for an efficient means of exploration. My immediate thought on the design is that rather than clicking a button to manually emit the crossfilter each time you want to apply an update, it might make more sense to have a setting where the time range is either emitted continuously... or not. Then any change to the time range/zoom would update the filter live, like the immediacy of clicking a pie slice. We may want to either just do that (without any button click) or add a toggle somewhere to enable/disable that crossfilter broadcast/updating/emission. Curious what Kasia thinks about all this :)

I’ve also pinged @Kamil Gabryjelski (Preset) and @Ville Brofeldt for a review on the PR itself, since they have a lot of experience in implementing crossfilters, and can take a closer look at the code.

Thank you for your contribution!

rusackas avatar Nov 30 '22 16:11 rusackas

Not sure the status of this PR after so long (is there still interest?), so I'll just close/open to kick-start the CI process and see how it does.

rusackas avatar Feb 06 '24 19:02 rusackas

Yes it's definitely something which would is useful for us, and a wider audience for sure.

@rusackas Evan you had previously mentioned having a couple Preset developers have a look over it?

fullergalway avatar Feb 08 '24 07:02 fullergalway

I'll try to ping some folks (e.g. @kgabryje ) about it who know the datamask implications better than I do, but I see a couple things right away that I'm curious about:

  1. There's a typescript error that will need to be addressed which will prevent merging.
  2. I see there's a vector path hard-coded in here, which raises a few questions:
  • Can we import/use the path from one of our SVG files (filter icon) so that it's always in sync if we change that?
  • Is this the right icon to use to emit a cross-filter, or is there something better (cc. @kasiazjc )
  • This might be getting too fancy, but... can/should the icon/button be togglable to emit/remove the crossfilter?

rusackas avatar Feb 08 '24 16:02 rusackas

Yes it's definitely something which would is useful for us, and a wider audience for sure.

You're quite right... I was just looking through older things on the repo, and didn't look closely enough at the PR. This is a cool feature, I hope we can get it across the finish line.

rusackas avatar Feb 08 '24 16:02 rusackas