Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fullergalway
Copy link

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:
  • 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

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.

@codecov
Copy link

codecov bot commented Nov 22, 2022

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%) ⬇️

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%) ⬇️

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

@fullergalway fullergalway changed the title [WIP] feat: support x-filter on echarts timeseries zoom feat: [WIP] support x-filter on echarts timeseries zoom Nov 22, 2022
@rusackas
Copy link
Member

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
Copy link
Member

rusackas commented Feb 6, 2024

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 rusackas closed this Feb 6, 2024
@rusackas rusackas reopened this Feb 6, 2024
@fullergalway
Copy link
Author

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?

@rusackas
Copy link
Member

rusackas commented Feb 8, 2024

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
Copy link
Member

rusackas commented Feb 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants