You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/11/21 15:05:31 UTC
[GitHub] [superset] fullergalway opened a new pull request, #22183: [WIP] feat: support x-filter on echarts timeseries zoom
fullergalway opened a new pull request, #22183:
URL: https://github.com/apache/superset/pull/22183
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
It would be nice to set a time range filter after using the zoom tool
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [X] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: [WIP] support x-filter on echarts timeseries zoom [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #22183:
URL: https://github.com/apache/superset/pull/22183#issuecomment-1934524689
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: [WIP] support x-filter on echarts timeseries zoom [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #22183: feat: [WIP] support x-filter on echarts timeseries zoom
URL: https://github.com/apache/superset/pull/22183
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: [WIP] support x-filter on echarts timeseries zoom [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #22183:
URL: https://github.com/apache/superset/pull/22183#issuecomment-1934521872
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #22183: [WIP] feat: support x-filter on echarts timeseries zoom
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22183:
URL: https://github.com/apache/superset/pull/22183#issuecomment-1323262761
# [Codecov](https://codecov.io/gh/apache/superset/pull/22183?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22183](https://codecov.io/gh/apache/superset/pull/22183?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (db8798a) into [master](https://codecov.io/gh/apache/superset/commit/9c52cca95a142df717a57eacf87627d81c1d23ad?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9c52cca) will **decrease** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #22183 +/- ##
==========================================
- Coverage 66.96% 66.95% -0.01%
==========================================
Files 1835 1835
Lines 69970 69980 +10
Branches 7590 7592 +2
==========================================
Hits 46856 46856
- Misses 21148 21157 +9
- Partials 1966 1967 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `53.78% <0.00%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22183?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...gin-chart-echarts/src/Timeseries/transformProps.ts](https://codecov.io/gh/apache/superset/pull/22183/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy90cmFuc2Zvcm1Qcm9wcy50cw==) | `46.39% <0.00%> (-5.34%)` | :arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #22183: feat: [WIP] support x-filter on echarts timeseries zoom
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #22183:
URL: https://github.com/apache/superset/pull/22183#issuecomment-1332453130
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: [WIP] support x-filter on echarts timeseries zoom [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #22183:
URL: https://github.com/apache/superset/pull/22183#issuecomment-1930635511
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: [WIP] support x-filter on echarts timeseries zoom [superset]
Posted by "fullergalway (via GitHub)" <gi...@apache.org>.
fullergalway commented on PR #22183:
URL: https://github.com/apache/superset/pull/22183#issuecomment-1933521664
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org