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/01/12 03:41:57 UTC
[GitHub] [superset] stevetracvc opened a new pull request #18009: feat: log scale and step size control for range filters
stevetracvc opened a new pull request #18009:
URL: https://github.com/apache/superset/pull/18009
### SUMMARY
This is my PR for issue Extra features for Range Filters #16582
Range filters are fantastic, but there is no simple way to adjust the step size or the scale of the range. Scale is useful for large ranges, eg a log scale, so that you can easily select small and large values
- Step Size is just an additional Advanced Config dropdown box with several pre-populated choices, as well as a free form option. The tricky part was getting it to properly work in the Native Filter Config modal, since they're expecting all parameters to be checkboxes.
- Scale is a little trickier, but it could possibly be set up as multiple options (eg log, power, etc). Currently I implemented a log scale option, which would make zeros hard. I'm currently ignoring that :)
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Config Screen Usage
Log Scale with 0.01 step size
Linear Scale with 100k step size
Config Screen Defaults
### 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 -->
- [x] Has associated issue: Fixes #16582
- [x] Required feature flags: DASHBOARD_NATIVE_FILTERS, DASHBOARD_CROSS_FILTERS
- [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
--
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] edited a comment on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1077042550
# [Codecov](https://codecov.io/gh/apache/superset/pull/18009?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 [#18009](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9d4f5a0) into [master](https://codecov.io/gh/apache/superset/commit/88029e21b6068f845d806cfc10d478a5d972ffa5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (88029e2) will **decrease** coverage by `0.03%`.
> The diff coverage is `52.63%`.
> :exclamation: Current head 9d4f5a0 differs from pull request most recent head a2cd0f0. Consider uploading reports for the commit a2cd0f0 to get more accurate results
```diff
@@ Coverage Diff @@
## master #18009 +/- ##
==========================================
- Coverage 66.57% 66.53% -0.04%
==========================================
Files 1667 1670 +3
Lines 64421 64452 +31
Branches 6503 6520 +17
==========================================
- Hits 42886 42885 -1
- Misses 19850 19868 +18
- Partials 1685 1699 +14
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.30% <52.63%> (-0.06%)` | :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/18009?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...set-frontend/src/filters/components/Range/types.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS90eXBlcy50cw==) | `33.33% <33.33%> (ø)` | |
| [...nfigModal/FiltersConfigForm/getControlItemsMap.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL2dldENvbnRyb2xJdGVtc01hcC50c3g=) | `71.11% <50.00%> (-7.68%)` | :arrow_down: |
| [...src/filters/components/Range/RangeFilterPlugin.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9SYW5nZUZpbHRlclBsdWdpbi50c3g=) | `68.00% <57.14%> (-5.59%)` | :arrow_down: |
| [...ntend/src/filters/components/Range/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9jb250cm9sUGFuZWwudHM=) | `100.00% <100.00%> (ø)` | |
| [...nd/src/components/MessageToasts/ToastPresenter.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVzc2FnZVRvYXN0cy9Ub2FzdFByZXNlbnRlci50c3g=) | `93.33% <0.00%> (-6.67%)` | :arrow_down: |
| [...d/src/dashboard/components/FiltersBadge/Styles.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0ZpbHRlcnNCYWRnZS9TdHlsZXMudHN4) | `86.36% <0.00%> (-5.53%)` | :arrow_down: |
| [...c/views/CRUD/data/database/DatabaseModal/styles.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL3N0eWxlcy50cw==) | `76.00% <0.00%> (-5.25%)` | :arrow_down: |
| [.../src/explore/components/DataTableControl/index.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9EYXRhVGFibGVDb250cm9sL2luZGV4LnRzeA==) | `71.62% <0.00%> (-2.67%)` | :arrow_down: |
| [superset-frontend/src/views/components/SubMenu.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NvbXBvbmVudHMvU3ViTWVudS50c3g=) | `87.50% <0.00%> (-1.87%)` | :arrow_down: |
| [superset-frontend/src/components/Chart/Chart.jsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvQ2hhcnQuanN4) | `52.45% <0.00%> (-1.64%)` | :arrow_down: |
| ... and [59 more](https://codecov.io/gh/apache/superset/pull/18009/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [88029e2...a2cd0f0](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] stevetracvc edited a comment on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
stevetracvc edited a comment on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1084594682
> Thanks for the improvement @stevetracvc! 1 issue that I spotted while testing - if the upper range value is smaller then `current value + step`, then we can't select it. For example, in the video my range is about [0, 41]. When I select step 25, then I can only select values 0 and 25. I think 41 should also be selectable, even if that would mean making smaller step. WDYT?
>
> Screen.Recording.2022-03-31.at.11.29.09.mov
I do think there are some issues with endpoints every now and then. I thought I fixed it for things like that (honestly I've never used anything bigger than 1 :laughing: ) especially with, eg, log scaling, when trying to drag to the end. Those were a rounding issue but this one isn't. I'll look around and see what I can figure out. The tests warn about the endpoints not being increments of the step size. What do you think would be the correct approach? Should the endpoint be **moved** to an increment of the step size? In your example, it would get moved to 50.
--
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] edited a comment on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1077042550
# [Codecov](https://codecov.io/gh/apache/superset/pull/18009?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 [#18009](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (209403d) into [master](https://codecov.io/gh/apache/superset/commit/88029e21b6068f845d806cfc10d478a5d972ffa5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (88029e2) will **decrease** coverage by `0.02%`.
> The diff coverage is `57.37%`.
```diff
@@ Coverage Diff @@
## master #18009 +/- ##
==========================================
- Coverage 66.57% 66.54% -0.03%
==========================================
Files 1667 1670 +3
Lines 64421 64456 +35
Branches 6503 6525 +22
==========================================
+ Hits 42886 42890 +4
- Misses 19850 19861 +11
- Partials 1685 1705 +20
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.31% <57.37%> (-0.05%)` | :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/18009?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...nfigModal/FiltersConfigForm/getControlItemsMap.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL2dldENvbnRyb2xJdGVtc01hcC50c3g=) | `71.11% <50.00%> (-7.68%)` | :arrow_down: |
| [...set-frontend/src/filters/components/Range/types.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS90eXBlcy50cw==) | `53.84% <53.84%> (ø)` | |
| [...src/filters/components/Range/RangeFilterPlugin.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9SYW5nZUZpbHRlclBsdWdpbi50c3g=) | `68.80% <60.00%> (-4.79%)` | :arrow_down: |
| [...ntend/src/filters/components/Range/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9jb250cm9sUGFuZWwudHM=) | `100.00% <100.00%> (ø)` | |
| [...nd/src/components/MessageToasts/ToastPresenter.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVzc2FnZVRvYXN0cy9Ub2FzdFByZXNlbnRlci50c3g=) | `93.33% <0.00%> (-6.67%)` | :arrow_down: |
| [...d/src/dashboard/components/FiltersBadge/Styles.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0ZpbHRlcnNCYWRnZS9TdHlsZXMudHN4) | `86.36% <0.00%> (-5.53%)` | :arrow_down: |
| [...c/views/CRUD/data/database/DatabaseModal/styles.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL3N0eWxlcy50cw==) | `76.00% <0.00%> (-5.25%)` | :arrow_down: |
| [.../src/explore/components/DataTableControl/index.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9EYXRhVGFibGVDb250cm9sL2luZGV4LnRzeA==) | `71.62% <0.00%> (-2.67%)` | :arrow_down: |
| [superset-frontend/src/views/components/SubMenu.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NvbXBvbmVudHMvU3ViTWVudS50c3g=) | `87.50% <0.00%> (-1.87%)` | :arrow_down: |
| [superset-frontend/src/components/Chart/Chart.jsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvQ2hhcnQuanN4) | `52.45% <0.00%> (-1.64%)` | :arrow_down: |
| ... and [59 more](https://codecov.io/gh/apache/superset/pull/18009/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [88029e2...209403d](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] stevetracvc commented on a change in pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
stevetracvc commented on a change in pull request #18009:
URL: https://github.com/apache/superset/pull/18009#discussion_r839603549
##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
##########
@@ -214,6 +216,69 @@ export default function getControlItemsMap({
);
mapControlItems[controlItem.name] = { element, checked: initialValue };
});
+ controlItems
+ .filter(
+ (controlItem: CustomControlItem) =>
+ controlItem?.config?.renderTrigger &&
+ controlItem.name !== 'sortAscending' &&
Review comment:
You mean and just leave it to checking if it's a SelectControl? I'm fine with that but honestly I never checked what those other conditions are for. I just duplicated the previous section and tweaked it for a select :laughing:
--
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] stevetracvc commented on a change in pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
stevetracvc commented on a change in pull request #18009:
URL: https://github.com/apache/superset/pull/18009#discussion_r783241837
##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
##########
@@ -211,6 +213,69 @@ export default function getControlItemsMap({
);
mapControlItems[controlItem.name] = { element, checked: initialValue };
});
+ controlItems
+ .filter(
+ (controlItem: CustomControlItem) =>
+ controlItem?.config?.renderTrigger &&
+ controlItem.name !== 'sortAscending' &&
+ controlItem?.config?.type === 'SelectControl',
+ )
+ .forEach(controlItem => {
+ const initialValue =
+ filterToEdit?.controlValues?.[controlItem.name] ??
+ controlItem?.config?.default;
+ const element = (
+ <>
+ <CleanFormItem
+ name={['filters', filterId, 'requiredFirst', controlItem.name]}
+ hidden
+ initialValue={
+ controlItem?.config?.requiredFirst && filterToEdit?.requiredFirst
+ }
+ />
+ <Tooltip
+ key={controlItem.name}
+ placement="left"
+ title={
+ controlItem.config.affectsDataMask &&
+ disabled &&
+ t('Populate "Default value" to enable this control')
+ }
+ >
+ <StyledRowFormItem
+ key={controlItem.name}
+ name={['filters', filterId, 'controlValues', controlItem.name]}
+ initialValue={initialValue}
+ valuePropName="option"
+ colon={false}
+ label={
+ <StyledLabel>
+ {t(`${controlItem.config?.label}`) || t('Select')}
+ </StyledLabel>
+ }
+ >
+ <SelectControl
+ name={controlItem.name}
+ clearable={false}
+ freeForm={controlItem.config.freeForm}
+ disabled={controlItem.config.affectsDataMask && disabled}
+ onChange={(value: any) => {
Review comment:
Good question @geido
So the Native Filter control didn't previously allow for a drop down, so as a new feature it's hard to imagine hoe else this could be used. I'm assuming string and number are likely the only types here, and I just forgot to change it from any.
--
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] github-actions[bot] commented on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1082162153
@geido Ephemeral environment spinning up at http://54.203.236.191:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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] kgabryje edited a comment on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
kgabryje edited a comment on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1084750513
> Should the endpoint be **moved** to an increment of the step size? In your example, it would get moved to 50.
I'd probably expect something like `min(maxRangeValue, currentValue + step)`, so for example with linear scale, step 25 and values in range [0,41], possible selections would be 0, 25, 41. But to be honest, the current behaviour _kinda_ makes sense too... @yousoph @jess-dillard what do you think?
--
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] geido commented on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
geido commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1081966494
/testenv up
--
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] geido commented on a change in pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
geido commented on a change in pull request #18009:
URL: https://github.com/apache/superset/pull/18009#discussion_r783185579
##########
File path: superset-frontend/src/filters/components/Range/controlPanel.ts
##########
@@ -69,6 +69,41 @@ const config: ControlPanelConfig = {
},
},
],
+ [
+ {
+ name: 'logScale',
+ config: {
+ type: 'CheckboxControl',
+ label: t('Logarithmic Scale'),
Review comment:
```suggestion
label: t('Logarithmic scale'),
```
##########
File path: superset-frontend/src/filters/components/Range/controlPanel.ts
##########
@@ -69,6 +69,41 @@ const config: ControlPanelConfig = {
},
},
],
+ [
+ {
+ name: 'logScale',
+ config: {
+ type: 'CheckboxControl',
+ label: t('Logarithmic Scale'),
+ default: false,
+ renderTrigger: true,
+ description: t('Make the scale logarithmic.'),
+ },
+ },
+ ],
+ [
+ {
+ name: 'stepSize',
+ config: {
+ type: 'SelectControl',
+ label: t('Step Size'),
Review comment:
```suggestion
label: t('Step size'),
```
##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
##########
@@ -211,6 +213,69 @@ export default function getControlItemsMap({
);
mapControlItems[controlItem.name] = { element, checked: initialValue };
});
+ controlItems
+ .filter(
+ (controlItem: CustomControlItem) =>
+ controlItem?.config?.renderTrigger &&
+ controlItem.name !== 'sortAscending' &&
+ controlItem?.config?.type === 'SelectControl',
+ )
+ .forEach(controlItem => {
+ const initialValue =
+ filterToEdit?.controlValues?.[controlItem.name] ??
+ controlItem?.config?.default;
+ const element = (
+ <>
+ <CleanFormItem
+ name={['filters', filterId, 'requiredFirst', controlItem.name]}
+ hidden
+ initialValue={
+ controlItem?.config?.requiredFirst && filterToEdit?.requiredFirst
+ }
+ />
+ <Tooltip
+ key={controlItem.name}
+ placement="left"
+ title={
+ controlItem.config.affectsDataMask &&
+ disabled &&
+ t('Populate "Default value" to enable this control')
+ }
+ >
+ <StyledRowFormItem
+ key={controlItem.name}
+ name={['filters', filterId, 'controlValues', controlItem.name]}
+ initialValue={initialValue}
+ valuePropName="option"
+ colon={false}
+ label={
+ <StyledLabel>
+ {t(`${controlItem.config?.label}`) || t('Select')}
+ </StyledLabel>
+ }
+ >
+ <SelectControl
+ name={controlItem.name}
+ clearable={false}
+ freeForm={controlItem.config.freeForm}
+ disabled={controlItem.config.affectsDataMask && disabled}
+ onChange={(value: any) => {
+ setNativeFilterFieldValues(form, filterId, {
+ [controlItem.name]: value,
+ defaultDataMask: null,
+ });
+ // controlItem.config.value = value;
Review comment:
```suggestion
```
##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
##########
@@ -211,6 +213,69 @@ export default function getControlItemsMap({
);
mapControlItems[controlItem.name] = { element, checked: initialValue };
});
+ controlItems
+ .filter(
+ (controlItem: CustomControlItem) =>
+ controlItem?.config?.renderTrigger &&
+ controlItem.name !== 'sortAscending' &&
+ controlItem?.config?.type === 'SelectControl',
+ )
+ .forEach(controlItem => {
+ const initialValue =
+ filterToEdit?.controlValues?.[controlItem.name] ??
+ controlItem?.config?.default;
+ const element = (
+ <>
+ <CleanFormItem
+ name={['filters', filterId, 'requiredFirst', controlItem.name]}
+ hidden
+ initialValue={
+ controlItem?.config?.requiredFirst && filterToEdit?.requiredFirst
+ }
+ />
+ <Tooltip
+ key={controlItem.name}
+ placement="left"
+ title={
+ controlItem.config.affectsDataMask &&
+ disabled &&
+ t('Populate "Default value" to enable this control')
+ }
+ >
+ <StyledRowFormItem
+ key={controlItem.name}
+ name={['filters', filterId, 'controlValues', controlItem.name]}
+ initialValue={initialValue}
+ valuePropName="option"
+ colon={false}
+ label={
+ <StyledLabel>
+ {t(`${controlItem.config?.label}`) || t('Select')}
+ </StyledLabel>
+ }
+ >
+ <SelectControl
+ name={controlItem.name}
+ clearable={false}
+ freeForm={controlItem.config.freeForm}
+ disabled={controlItem.config.affectsDataMask && disabled}
+ onChange={(value: any) => {
Review comment:
Do you think we can be more specific for the type of the `value` of this `onChange` event?
--
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] kgabryje commented on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1084750513
> Should the endpoint be **moved** to an increment of the step size? In your example, it would get moved to 50.
I'd probably expect something like `min(maxRangeValue, currentValue + step)`, so for example with linear scale, step 25 and values in range [0,41], possible selections would be 0, 25, 41. But to be honest, the current behaviour _kinda_ makes sense too... @yousoph @jess-dillard what do you think?
--
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] kgabryje commented on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1084752196
> But you can clearly see that's not the order the modal decides to put stuff. enableEmptyFilter is the last control in the modal, yet the first in the controlSetRows array. I still don't get exactly how that controlSetRows should be structured, as I would have thought the enableEmptyFilter and enableSingleValue controls would be on the same row (since they're in the same controlSetRow) which is (I believe) how it works for visualization controlPanel configs.
I'm not very familiar with native filters control panels... @michael-s-molina @geido do you guys have some context on this?
--
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] jess-dillard edited a comment on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
jess-dillard edited a comment on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1084861204
> I'd probably expect something like min(maxRangeValue, currentValue + step), so for example with linear scale, step 25 and values in range [0,41], possible selections would be 0, 25, 41. But to be honest, the current behaviour kinda makes sense too... @yousoph @jess-dillard what do you think?
@kgabryje I agree with you – I would also expect the possible selections to be 0, 25, and 41 in that scenario.
--
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] stevetracvc commented on a change in pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
stevetracvc commented on a change in pull request #18009:
URL: https://github.com/apache/superset/pull/18009#discussion_r840100383
##########
File path: superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx
##########
@@ -186,41 +218,62 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) {
}
return {
- lower: lowerRaw > min ? lowerRaw : null,
- upper: upperRaw < max ? upperRaw : null,
+ lower: lowerRaw > Number(transformScale(min)) ? lowerRaw : null,
+ upper: upperRaw < Number(transformScale(max)) ? upperRaw : null,
};
},
- [max, min, enableSingleExactValue],
+ [max, min, transformScale, value, enableSingleExactValue],
);
const handleAfterChange = useCallback(
(value: [number, number]): void => {
- setValue(value);
- const { lower, upper } = getBounds(value);
+ let val = value;
+ if (value[0] === min && value[1] === max) {
+ // after a filter value reset, make sure it's a transformed value
+ val = [transformScale(value[0]), transformScale(value[1])];
+ }
+ // antd apparently uses the floor value, not the rounded value...?
+ // which causes issues like log(123) = 2.0899
+ if (val[1] === Math.floor(transformScale(max) / stepSize) * stepSize) {
+ val = [val[0], transformScale(max)];
+ }
+ if (val[0] === Math.floor(transformScale(min) / stepSize) * stepSize) {
+ val = [transformScale(min), val[1]];
+ }
+ // value is transformed
+ setValue(val);
+ // lower & upper are transformed
+ const { lower, upper } = getBounds(val);
setMarks(getMarks(lower, upper));
-
+ // removed Number
setDataMask({
- extraFormData: getRangeExtraFormData(col, lower, upper),
+ extraFormData: getRangeExtraFormData(
+ col,
+ inverseScale(lower),
+ inverseScale(upper),
+ ),
filterState: {
value: lower !== null || upper !== null ? value : null,
Review comment:
What does the filterState.value do? The variable "value" is transformed, meaning it could be the log of the actual desired value. So I'd expect that this would need to actually be
`value.map(inverseScale)`
like how it is below for the lower and upper vars. But the queries appear to run properly, and the text labels are correct.... :thinking:
--
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] stevetracvc commented on a change in pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
stevetracvc commented on a change in pull request #18009:
URL: https://github.com/apache/superset/pull/18009#discussion_r841104217
##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
##########
@@ -214,6 +216,69 @@ export default function getControlItemsMap({
);
mapControlItems[controlItem.name] = { element, checked: initialValue };
});
+ controlItems
+ .filter(
+ (controlItem: CustomControlItem) =>
+ controlItem?.config?.renderTrigger &&
+ controlItem.name !== 'sortAscending' &&
Review comment:
renderTrigger is needed, otherwise the COLUMN select box shows up again. sortAscending isn't though.
--
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] stevetracvc commented on a change in pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
stevetracvc commented on a change in pull request #18009:
URL: https://github.com/apache/superset/pull/18009#discussion_r783250114
##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
##########
@@ -211,6 +213,69 @@ export default function getControlItemsMap({
);
mapControlItems[controlItem.name] = { element, checked: initialValue };
});
+ controlItems
+ .filter(
+ (controlItem: CustomControlItem) =>
+ controlItem?.config?.renderTrigger &&
+ controlItem.name !== 'sortAscending' &&
+ controlItem?.config?.type === 'SelectControl',
+ )
+ .forEach(controlItem => {
+ const initialValue =
+ filterToEdit?.controlValues?.[controlItem.name] ??
+ controlItem?.config?.default;
+ const element = (
+ <>
+ <CleanFormItem
+ name={['filters', filterId, 'requiredFirst', controlItem.name]}
+ hidden
+ initialValue={
+ controlItem?.config?.requiredFirst && filterToEdit?.requiredFirst
+ }
+ />
+ <Tooltip
+ key={controlItem.name}
+ placement="left"
+ title={
+ controlItem.config.affectsDataMask &&
+ disabled &&
+ t('Populate "Default value" to enable this control')
+ }
+ >
+ <StyledRowFormItem
+ key={controlItem.name}
+ name={['filters', filterId, 'controlValues', controlItem.name]}
+ initialValue={initialValue}
+ valuePropName="option"
+ colon={false}
+ label={
+ <StyledLabel>
+ {t(`${controlItem.config?.label}`) || t('Select')}
+ </StyledLabel>
+ }
+ >
+ <SelectControl
+ name={controlItem.name}
+ clearable={false}
+ freeForm={controlItem.config.freeForm}
+ disabled={controlItem.config.affectsDataMask && disabled}
+ onChange={(value: any) => {
Review comment:
I can see changing the "logarithmic scale" from a checkbox to a drop down, with choices for linear, power, log, symlog (would need an input box for the symlog constant), maybe others. I'm using d3-scale, which has those built in:
https://github.com/d3/d3-scale#continuous-scales
--
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] edited a comment on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1077042550
# [Codecov](https://codecov.io/gh/apache/superset/pull/18009?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 [#18009](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9d4f5a0) into [master](https://codecov.io/gh/apache/superset/commit/88029e21b6068f845d806cfc10d478a5d972ffa5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (88029e2) will **decrease** coverage by `0.03%`.
> The diff coverage is `52.63%`.
> :exclamation: Current head 9d4f5a0 differs from pull request most recent head a2cd0f0. Consider uploading reports for the commit a2cd0f0 to get more accurate results
```diff
@@ Coverage Diff @@
## master #18009 +/- ##
==========================================
- Coverage 66.57% 66.53% -0.04%
==========================================
Files 1667 1670 +3
Lines 64421 64452 +31
Branches 6503 6520 +17
==========================================
- Hits 42886 42885 -1
- Misses 19850 19868 +18
- Partials 1685 1699 +14
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.30% <52.63%> (-0.06%)` | :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/18009?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...set-frontend/src/filters/components/Range/types.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS90eXBlcy50cw==) | `33.33% <33.33%> (ø)` | |
| [...nfigModal/FiltersConfigForm/getControlItemsMap.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL2dldENvbnRyb2xJdGVtc01hcC50c3g=) | `71.11% <50.00%> (-7.68%)` | :arrow_down: |
| [...src/filters/components/Range/RangeFilterPlugin.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9SYW5nZUZpbHRlclBsdWdpbi50c3g=) | `68.00% <57.14%> (-5.59%)` | :arrow_down: |
| [...ntend/src/filters/components/Range/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9jb250cm9sUGFuZWwudHM=) | `100.00% <100.00%> (ø)` | |
| [...nd/src/components/MessageToasts/ToastPresenter.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVzc2FnZVRvYXN0cy9Ub2FzdFByZXNlbnRlci50c3g=) | `93.33% <0.00%> (-6.67%)` | :arrow_down: |
| [...d/src/dashboard/components/FiltersBadge/Styles.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0ZpbHRlcnNCYWRnZS9TdHlsZXMudHN4) | `86.36% <0.00%> (-5.53%)` | :arrow_down: |
| [...c/views/CRUD/data/database/DatabaseModal/styles.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL3N0eWxlcy50cw==) | `76.00% <0.00%> (-5.25%)` | :arrow_down: |
| [.../src/explore/components/DataTableControl/index.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9EYXRhVGFibGVDb250cm9sL2luZGV4LnRzeA==) | `71.62% <0.00%> (-2.67%)` | :arrow_down: |
| [superset-frontend/src/views/components/SubMenu.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NvbXBvbmVudHMvU3ViTWVudS50c3g=) | `87.50% <0.00%> (-1.87%)` | :arrow_down: |
| [superset-frontend/src/components/Chart/Chart.jsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvQ2hhcnQuanN4) | `52.45% <0.00%> (-1.64%)` | :arrow_down: |
| ... and [59 more](https://codecov.io/gh/apache/superset/pull/18009/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [88029e2...a2cd0f0](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] kgabryje commented on a change in pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #18009:
URL: https://github.com/apache/superset/pull/18009#discussion_r839387677
##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
##########
@@ -214,6 +216,69 @@ export default function getControlItemsMap({
);
mapControlItems[controlItem.name] = { element, checked: initialValue };
});
+ controlItems
+ .filter(
+ (controlItem: CustomControlItem) =>
+ controlItem?.config?.renderTrigger &&
+ controlItem.name !== 'sortAscending' &&
Review comment:
We probably don't need this check?
--
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] stevetracvc commented on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
stevetracvc commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1086696409
Awesome! Questions inline:
>
> We had a conversation with @michael-s-molina who has suggested the following:
>
> * The easiest solution is just to leave it in the current position if we can live with it (of course)
> * Another way is to move the code directly into `FiltersConfigForm` between the description and the default value
If we did that, then that moves more of the config out of the specific native filter component's controlPanel file and into the main one. I had tried to make this more general so that other filter types could also include select boxes for controls if needed. If you do want the FiltersConfigForm modified instead, it seems like it should go under the "single value" checkbox, as part of the configuration rather than the settings. What do you think?
>
> You are right this wasn't straightforward forward but we aim to improve the codebase of native filters in the upcoming phase 2.
Since that's the plan, which route would be better? It seems like we'd want everything in the specific component's controlPanel rather than spread across the two files. And maybe revisit this when phase 2 is complete?
>
> Some other observations that came out from our conversation:
>
> * This functionality should be under a checkbox as it’s very specific, such as “Customise default step scaling function”
Is it really that specific? Am I one of the few people using range filters for a large range of values? If you want it hidden like that, then the wording on that needs improvement, since step size and scaling can each be used independent of the other one. Eg, you can't currently use a range filter for floats since the step size is inherently 1 (especially on a range 0 to 1). I feel like each one is its own thing, and step size could be incorporated without bothering with the scaling.
> * The inputs should not take the full width of the Modal. Please follow the same UI of existing inputs
I finally found where it does its magic, so I made them half width now (I haven't pushed yet). I'm starting to understand how everything works here, but a lot of what I originally did was just get it to work because these were features I needed...so I copied and pasted code.
The issue is that FiltersControlModal calls getControlItemsMap, which calls getControlItems, which **flattens** the controlPanel config array. I think ideally, step size and scaling function would be in the same row, don't you think? But that would take a decent rework of the FiltersConfigModal code to make that work.
> * If the step size is lower than two decimals, the exact number of decimals should also be visible in the left sidebar, otherwise it’s impossible to see those decimals changing.
Not sure I follow on this one, though it also might be related to the issue of values not being in exact increments of the step size. When I have the step size at 0.001, linear scale, for something with values 0 to 1, each increment is actually 0.005, which I think is a limitation on the antd slider control.
>
> Let me know if you have any questions. Thanks for the hard work!
--
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] kgabryje commented on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1084323798
Thanks for the improvement @stevetracvc!
1 issue that I spotted while testing - if the upper range value is smaller then `current value + step`, then we can't select it. For example, in the video my range is about [0, 41]. When I select step 25, then I can only select values 0 and 25. I think 41 should also be selectable, even if that would mean making smaller step. WDYT?
https://user-images.githubusercontent.com/15073128/161023937-9bfd852a-5ffd-4b0c-8c66-dd30da52d725.mov
--
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] stevetracvc commented on a change in pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
stevetracvc commented on a change in pull request #18009:
URL: https://github.com/apache/superset/pull/18009#discussion_r839604008
##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
##########
@@ -149,6 +150,7 @@ export default function getControlItemsMap({
(controlItem: CustomControlItem) =>
controlItem?.config?.renderTrigger &&
controlItem.name !== 'sortAscending' &&
+ controlItem?.config?.type !== 'SelectControl' &&
Review comment:
Why a checkbox? I added a whole new section (below) to handle dropdown selects because that doesn't exist anywhere else in the config modal. It's for the step size and scaling function select boxes
--
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] stevetracvc commented on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
stevetracvc commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1084594682
> Thanks for the improvement @stevetracvc! 1 issue that I spotted while testing - if the upper range value is smaller then `current value + step`, then we can't select it. For example, in the video my range is about [0, 41]. When I select step 25, then I can only select values 0 and 25. I think 41 should also be selectable, even if that would mean making smaller step. WDYT?
>
> Screen.Recording.2022-03-31.at.11.29.09.mov
I do think there are some issues with endpoints every now and then. I thought I fixed it for things like that (honestly I've never used anything bigger than 1 :laughing: ) especially with, eg, log scaling, when trying to drag to the end. Those were a rounding issue but this one isn't. I'll look around and see what I can figure out. The tests warn about the endpoints not being increments of the step size. What do you think would be the correct approach? Should the endpoint be **moved**? In your example, it would get moved to 50.
--
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] kasiazjc commented on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
kasiazjc commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1084750118
> > We might want to rearrange the controls. Clicking on "Filter has default value" triggers displaying default value control above the step controls, while step controls affect how default value picker works - that probably means that step controls should be above default value. <img alt="image" width="631" src="https://user-images.githubusercontent.com/15073128/161026010-8ac3b3d5-f271-44bb-9581-4bfc5233655e.png">
> > CC @kasiazjc
>
> That's an interesting observation but I have no idea how to do that. The default value stuff code is handled somewhere else, and honestly I'm not sure how it decides on the location of controlPanel components. In the controlPanel code for the Range Filter, I have
>
> ```
> {
> label: t('UI Configuration'),
> expanded: true,
> controlSetRows: [
> [
> {
> name: 'enableEmptyFilter',
> config: {
> type: 'CheckboxControl',
> label: t('Filter value is required'),
> default: false,
> renderTrigger: true,
> description: t(
> 'User must select a value before applying the filter',
> ),
> },
> },
> {
> name: 'enableSingleValue',
> config: {
> type: 'CheckboxControl',
> label: t('Single value'),
> default: SingleValueType.Exact,
> renderTrigger: true,
> description: t('Use only a single value.'),
> },
> },
> ],
> [
> {
> name: 'scaling',
> config: {
> type: 'SelectControl',
> label: t('Scaling Function'),
> default: PluginFilterRangeScalingFunctions.LINEAR,
> renderTrigger: true,
> freeForm: false,
> choices: Object.keys(
> SCALING_FUNCTION_ENUM_TO_SCALING_FUNCTION,
> ).map(key => [
> key,
> SCALING_FUNCTION_ENUM_TO_SCALING_FUNCTION[key].display,
> ]),
> description: t('Choose a scaling function for the slider.'),
> },
> },
> {
> name: 'stepSize',
> config: {
> type: 'SelectControl',
> label: t('Step Size'),
> default: 1,
> renderTrigger: true,
> freeForm: true,
> choices: [
> [0.001, 0.001],
> [0.01, 0.01],
> [0.1, 0.1],
> [1, 1],
> [2, 2],
> [10, 10],
> [25, 25],
> [100, 100],
> ],
> description: t('Set the slider step size.'),
> },
> },
> ],
> ],
> },
> ```
>
> But you can clearly see that's not the order the modal decides to put stuff. enableEmptyFilter is the last control in the modal, yet the first in the controlSetRows array. I still don't get exactly how that controlSetRows should be structured, as I would have thought the enableEmptyFilter and enableSingleValue controls would be on the same row (since they're in the same controlSetRow) which is (I believe) how it works for visualization controlPanel configs.
I agree with @kgabryje that it makes more sense to reorder the controls, so that the flow is more natural. I think it should be in that case:
- scaling unction
- Step size
- Filter has default value
Thanks @stevetracvc 🙏
--
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] jess-dillard commented on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
jess-dillard commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1084861204
>
@kgabryje I agree with you – I would also expect the possible selections to be 0, 25, and 41 in that scenario.
--
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 #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1017126677
Fixes #17043
--
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] stevetracvc commented on a change in pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
stevetracvc commented on a change in pull request #18009:
URL: https://github.com/apache/superset/pull/18009#discussion_r840100383
##########
File path: superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx
##########
@@ -186,41 +218,62 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) {
}
return {
- lower: lowerRaw > min ? lowerRaw : null,
- upper: upperRaw < max ? upperRaw : null,
+ lower: lowerRaw > Number(transformScale(min)) ? lowerRaw : null,
+ upper: upperRaw < Number(transformScale(max)) ? upperRaw : null,
};
},
- [max, min, enableSingleExactValue],
+ [max, min, transformScale, value, enableSingleExactValue],
);
const handleAfterChange = useCallback(
(value: [number, number]): void => {
- setValue(value);
- const { lower, upper } = getBounds(value);
+ let val = value;
+ if (value[0] === min && value[1] === max) {
+ // after a filter value reset, make sure it's a transformed value
+ val = [transformScale(value[0]), transformScale(value[1])];
+ }
+ // antd apparently uses the floor value, not the rounded value...?
+ // which causes issues like log(123) = 2.0899
+ if (val[1] === Math.floor(transformScale(max) / stepSize) * stepSize) {
+ val = [val[0], transformScale(max)];
+ }
+ if (val[0] === Math.floor(transformScale(min) / stepSize) * stepSize) {
+ val = [transformScale(min), val[1]];
+ }
+ // value is transformed
+ setValue(val);
+ // lower & upper are transformed
+ const { lower, upper } = getBounds(val);
setMarks(getMarks(lower, upper));
-
+ // removed Number
setDataMask({
- extraFormData: getRangeExtraFormData(col, lower, upper),
+ extraFormData: getRangeExtraFormData(
+ col,
+ inverseScale(lower),
+ inverseScale(upper),
+ ),
filterState: {
value: lower !== null || upper !== null ? value : null,
Review comment:
@michael-s-molina What does the filterState.value do? The variable "value" is transformed, meaning it could be the log of the actual desired value. So I'd expect that this would need to actually be
`value.map(inverseScale)`
like how it is below for the lower and upper vars. But the queries appear to run properly, and the text labels are correct.... :thinking: I don't see any difference when I inverse this value or leave it transformed.
--
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] kgabryje commented on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1084334268
We might want to rearrange the controls. Clicking on "Filter has default value" below the step controls triggers displaying default value control above the step controls, which might be confusing. Also, the values chosen in step controls affect how the default value control works, which would probably mean that step controls should be placed above default value control
<img width="631" alt="image" src="https://user-images.githubusercontent.com/15073128/161026010-8ac3b3d5-f271-44bb-9581-4bfc5233655e.png">
CC @kasiazjc
--
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] stevetracvc edited a comment on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
stevetracvc edited a comment on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1084583734
> We might want to rearrange the controls. Clicking on "Filter has default value" triggers displaying default value control above the step controls, while step controls affect how default value picker works - that probably means that step controls should be above default value. <img alt="image" width="631" src="https://user-images.githubusercontent.com/15073128/161026010-8ac3b3d5-f271-44bb-9581-4bfc5233655e.png">
>
> CC @kasiazjc
That's an interesting observation but I have no idea how to do that. The default value stuff code is handled somewhere else, and honestly I'm not sure how it decides on the location of controlPanel components. In the controlPanel code for the Range Filter, I have
```
{
label: t('UI Configuration'),
expanded: true,
controlSetRows: [
[
{
name: 'enableEmptyFilter',
config: {
type: 'CheckboxControl',
label: t('Filter value is required'),
default: false,
renderTrigger: true,
description: t(
'User must select a value before applying the filter',
),
},
},
{
name: 'enableSingleValue',
config: {
type: 'CheckboxControl',
label: t('Single value'),
default: SingleValueType.Exact,
renderTrigger: true,
description: t('Use only a single value.'),
},
},
],
[
{
name: 'scaling',
config: {
type: 'SelectControl',
label: t('Scaling Function'),
default: PluginFilterRangeScalingFunctions.LINEAR,
renderTrigger: true,
freeForm: false,
choices: Object.keys(
SCALING_FUNCTION_ENUM_TO_SCALING_FUNCTION,
).map(key => [
key,
SCALING_FUNCTION_ENUM_TO_SCALING_FUNCTION[key].display,
]),
description: t('Choose a scaling function for the slider.'),
},
},
{
name: 'stepSize',
config: {
type: 'SelectControl',
label: t('Step Size'),
default: 1,
renderTrigger: true,
freeForm: true,
choices: [
[0.001, 0.001],
[0.01, 0.01],
[0.1, 0.1],
[1, 1],
[2, 2],
[10, 10],
[25, 25],
[100, 100],
],
description: t('Set the slider step size.'),
},
},
],
],
},
```
But you can clearly see that's not the order the modal decides to put stuff. enableEmptyFilter is the last control in the modal, yet the first in the controlSetRows array. I still don't get exactly how that controlSetRows should be structured, as I would have thought the enableEmptyFilter and enableSingleValue controls would be on the same row (since they're in the same controlSetRow) which is (I believe) how it works for visualization controlPanel configs.
--
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 #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1077042550
# [Codecov](https://codecov.io/gh/apache/superset/pull/18009?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 [#18009](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9d4f5a0) into [master](https://codecov.io/gh/apache/superset/commit/88029e21b6068f845d806cfc10d478a5d972ffa5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (88029e2) will **decrease** coverage by `0.03%`.
> The diff coverage is `52.63%`.
```diff
@@ Coverage Diff @@
## master #18009 +/- ##
==========================================
- Coverage 66.57% 66.53% -0.04%
==========================================
Files 1667 1670 +3
Lines 64421 64452 +31
Branches 6503 6520 +17
==========================================
- Hits 42886 42885 -1
- Misses 19850 19868 +18
- Partials 1685 1699 +14
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.30% <52.63%> (-0.06%)` | :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/18009?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...set-frontend/src/filters/components/Range/types.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS90eXBlcy50cw==) | `33.33% <33.33%> (ø)` | |
| [...nfigModal/FiltersConfigForm/getControlItemsMap.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL2dldENvbnRyb2xJdGVtc01hcC50c3g=) | `71.11% <50.00%> (-7.68%)` | :arrow_down: |
| [...src/filters/components/Range/RangeFilterPlugin.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9SYW5nZUZpbHRlclBsdWdpbi50c3g=) | `68.00% <57.14%> (-5.59%)` | :arrow_down: |
| [...ntend/src/filters/components/Range/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9jb250cm9sUGFuZWwudHM=) | `100.00% <100.00%> (ø)` | |
| [...nd/src/components/MessageToasts/ToastPresenter.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVzc2FnZVRvYXN0cy9Ub2FzdFByZXNlbnRlci50c3g=) | `93.33% <0.00%> (-6.67%)` | :arrow_down: |
| [...d/src/dashboard/components/FiltersBadge/Styles.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0ZpbHRlcnNCYWRnZS9TdHlsZXMudHN4) | `86.36% <0.00%> (-5.53%)` | :arrow_down: |
| [...c/views/CRUD/data/database/DatabaseModal/styles.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL3N0eWxlcy50cw==) | `76.00% <0.00%> (-5.25%)` | :arrow_down: |
| [.../src/explore/components/DataTableControl/index.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9EYXRhVGFibGVDb250cm9sL2luZGV4LnRzeA==) | `71.62% <0.00%> (-2.67%)` | :arrow_down: |
| [superset-frontend/src/views/components/SubMenu.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NvbXBvbmVudHMvU3ViTWVudS50c3g=) | `87.50% <0.00%> (-1.87%)` | :arrow_down: |
| [superset-frontend/src/components/Chart/Chart.jsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvQ2hhcnQuanN4) | `52.45% <0.00%> (-1.64%)` | :arrow_down: |
| ... and [59 more](https://codecov.io/gh/apache/superset/pull/18009/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [88029e2...9d4f5a0](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] geido commented on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
geido commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1018541075
@stevetracvc I see that some tests are requiring some love. Let me know how I can help to move this forward! Thank you
--
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] edited a comment on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1077042550
# [Codecov](https://codecov.io/gh/apache/superset/pull/18009?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 [#18009](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (209403d) into [master](https://codecov.io/gh/apache/superset/commit/88029e21b6068f845d806cfc10d478a5d972ffa5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (88029e2) will **decrease** coverage by `0.02%`.
> The diff coverage is `57.37%`.
```diff
@@ Coverage Diff @@
## master #18009 +/- ##
==========================================
- Coverage 66.57% 66.54% -0.03%
==========================================
Files 1667 1670 +3
Lines 64421 64456 +35
Branches 6503 6525 +22
==========================================
+ Hits 42886 42890 +4
- Misses 19850 19861 +11
- Partials 1685 1705 +20
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.31% <57.37%> (-0.05%)` | :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/18009?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...nfigModal/FiltersConfigForm/getControlItemsMap.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL2dldENvbnRyb2xJdGVtc01hcC50c3g=) | `71.11% <50.00%> (-7.68%)` | :arrow_down: |
| [...set-frontend/src/filters/components/Range/types.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS90eXBlcy50cw==) | `53.84% <53.84%> (ø)` | |
| [...src/filters/components/Range/RangeFilterPlugin.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9SYW5nZUZpbHRlclBsdWdpbi50c3g=) | `68.80% <60.00%> (-4.79%)` | :arrow_down: |
| [...ntend/src/filters/components/Range/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9jb250cm9sUGFuZWwudHM=) | `100.00% <100.00%> (ø)` | |
| [...nd/src/components/MessageToasts/ToastPresenter.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVzc2FnZVRvYXN0cy9Ub2FzdFByZXNlbnRlci50c3g=) | `93.33% <0.00%> (-6.67%)` | :arrow_down: |
| [...d/src/dashboard/components/FiltersBadge/Styles.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0ZpbHRlcnNCYWRnZS9TdHlsZXMudHN4) | `86.36% <0.00%> (-5.53%)` | :arrow_down: |
| [...c/views/CRUD/data/database/DatabaseModal/styles.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL3N0eWxlcy50cw==) | `76.00% <0.00%> (-5.25%)` | :arrow_down: |
| [.../src/explore/components/DataTableControl/index.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9EYXRhVGFibGVDb250cm9sL2luZGV4LnRzeA==) | `71.62% <0.00%> (-2.67%)` | :arrow_down: |
| [superset-frontend/src/views/components/SubMenu.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NvbXBvbmVudHMvU3ViTWVudS50c3g=) | `87.50% <0.00%> (-1.87%)` | :arrow_down: |
| [superset-frontend/src/components/Chart/Chart.jsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvQ2hhcnQuanN4) | `52.45% <0.00%> (-1.64%)` | :arrow_down: |
| ... and [59 more](https://codecov.io/gh/apache/superset/pull/18009/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [88029e2...209403d](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] codecov[bot] edited a comment on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1077042550
# [Codecov](https://codecov.io/gh/apache/superset/pull/18009?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 [#18009](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e1e7399) into [master](https://codecov.io/gh/apache/superset/commit/88029e21b6068f845d806cfc10d478a5d972ffa5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (88029e2) will **increase** coverage by `0.09%`.
> The diff coverage is `84.41%`.
> :exclamation: Current head e1e7399 differs from pull request most recent head 7af88c4. Consider uploading reports for the commit 7af88c4 to get more accurate results
```diff
@@ Coverage Diff @@
## master #18009 +/- ##
==========================================
+ Coverage 66.57% 66.66% +0.09%
==========================================
Files 1667 1677 +10
Lines 64421 64761 +340
Branches 6503 6529 +26
==========================================
+ Hits 42886 43174 +288
- Misses 19850 19883 +33
- Partials 1685 1704 +19
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.32% <68.96%> (-0.04%)` | :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/18009?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...erset-ui-chart-controls/src/components/Tooltip.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL2NvbXBvbmVudHMvVG9vbHRpcC50c3g=) | `80.00% <ø> (ø)` | |
| [...et-ui-chart-controls/src/shared-controls/index.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9pbmRleC50c3g=) | `36.19% <0.00%> (-0.35%)` | :arrow_down: |
| [...perset-ui-chart-controls/src/utils/D3Formatting.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3V0aWxzL0QzRm9ybWF0dGluZy50cw==) | `100.00% <ø> (ø)` | |
| [...s/superset-ui-core/src/query/api/v1/handleError.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvYXBpL3YxL2hhbmRsZUVycm9yLnRz) | `100.00% <ø> (ø)` | |
| [...superset-ui-core/src/query/types/PostProcessing.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvUG9zdFByb2Nlc3NpbmcudHM=) | `100.00% <ø> (ø)` | |
| [.../superset-ui-core/src/query/types/QueryFormData.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvUXVlcnlGb3JtRGF0YS50cw==) | `100.00% <ø> (ø)` | |
| [...tend/packages/superset-ui-core/src/style/index.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvc3R5bGUvaW5kZXgudHN4) | `100.00% <ø> (ø)` | |
| [...ins/legacy-plugin-chart-sankey/src/ReactSankey.jsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LXNhbmtleS9zcmMvUmVhY3RTYW5rZXkuanN4) | `0.00% <ø> (ø)` | |
| [...lugin-chart-echarts/src/BigNumber/BigNumberViz.tsx](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQmlnTnVtYmVyL0JpZ051bWJlclZpei50c3g=) | `0.00% <ø> (ø)` | |
| [...src/BigNumber/BigNumberWithTrendline/buildQuery.ts](https://codecov.io/gh/apache/superset/pull/18009/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQmlnTnVtYmVyL0JpZ051bWJlcldpdGhUcmVuZGxpbmUvYnVpbGRRdWVyeS50cw==) | `11.11% <ø> (+2.02%)` | :arrow_up: |
| ... and [153 more](https://codecov.io/gh/apache/superset/pull/18009/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [88029e2...7af88c4](https://codecov.io/gh/apache/superset/pull/18009?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] stevetracvc commented on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
stevetracvc commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1084583734
> We might want to rearrange the controls. Clicking on "Filter has default value" triggers displaying default value control above the step controls, while step controls affect how default value picker works - that probably means that step controls should be above default value. <img alt="image" width="631" src="https://user-images.githubusercontent.com/15073128/161026010-8ac3b3d5-f271-44bb-9581-4bfc5233655e.png">
>
> CC @kasiazjc
That's an interesting observation but I have no idea how to do that. The default value stuff code is handled somewhere else, and honestly I'm not sure how it decides on the location of controlPanel components. In the controlPanel code for the Range Filter, I have
```
{
label: t('UI Configuration'),
expanded: true,
controlSetRows: [
[
{
name: 'enableEmptyFilter',
config: {
type: 'CheckboxControl',
label: t('Filter value is required'),
default: false,
renderTrigger: true,
description: t(
'User must select a value before applying the filter',
),
},
},
{
name: 'enableSingleValue',
config: {
type: 'CheckboxControl',
label: t('Single value'),
default: SingleValueType.Exact,
renderTrigger: true,
description: t('Use only a single value.'),
},
},
],
[
{
name: 'scaling',
config: {
type: 'SelectControl',
label: t('Scaling Function'),
default: PluginFilterRangeScalingFunctions.LINEAR,
renderTrigger: true,
freeForm: false,
choices: Object.keys(
SCALING_FUNCTION_ENUM_TO_SCALING_FUNCTION,
).map(key => [
key,
SCALING_FUNCTION_ENUM_TO_SCALING_FUNCTION[key].display,
]),
description: t('Choose a scaling function for the slider.'),
},
},
{
name: 'stepSize',
config: {
type: 'SelectControl',
label: t('Step Size'),
default: 1,
renderTrigger: true,
freeForm: true,
choices: [
[0.001, 0.001],
[0.01, 0.01],
[0.1, 0.1],
[1, 1],
[2, 2],
[10, 10],
[25, 25],
[100, 100],
],
description: t('Set the slider step size.'),
},
},
],
],
},
```
But you can clearly see that's not the order the modal decides to put stuff.
--
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] kgabryje edited a comment on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
kgabryje edited a comment on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1084334268
We might want to rearrange the controls. Clicking on "Filter has default value" triggers displaying default value control above the step controls, while step controls affect how default value picker works - that probably means that step controls should be above default value.
<img width="631" alt="image" src="https://user-images.githubusercontent.com/15073128/161026010-8ac3b3d5-f271-44bb-9581-4bfc5233655e.png">
CC @kasiazjc
--
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] kgabryje commented on a change in pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #18009:
URL: https://github.com/apache/superset/pull/18009#discussion_r839386326
##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
##########
@@ -149,6 +150,7 @@ export default function getControlItemsMap({
(controlItem: CustomControlItem) =>
controlItem?.config?.renderTrigger &&
controlItem.name !== 'sortAscending' &&
+ controlItem?.config?.type !== 'SelectControl' &&
Review comment:
I think that we should rather if the type is `Checkbox` rather than `not SelectControl`
##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
##########
@@ -149,6 +150,7 @@ export default function getControlItemsMap({
(controlItem: CustomControlItem) =>
controlItem?.config?.renderTrigger &&
controlItem.name !== 'sortAscending' &&
+ controlItem?.config?.type !== 'SelectControl' &&
Review comment:
I think that we should rather check if the type is `Checkbox` rather than `not SelectControl`
##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
##########
@@ -149,6 +150,7 @@ export default function getControlItemsMap({
(controlItem: CustomControlItem) =>
controlItem?.config?.renderTrigger &&
controlItem.name !== 'sortAscending' &&
+ controlItem?.config?.type !== 'SelectControl' &&
Review comment:
I think that we should check if the type is `Checkbox` rather than `not SelectControl`
--
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] geido commented on pull request #18009: feat: log scale and step size control for range filters [WIP]
Posted by GitBox <gi...@apache.org>.
geido commented on pull request #18009:
URL: https://github.com/apache/superset/pull/18009#issuecomment-1085898918
> > We might want to rearrange the controls. Clicking on "Filter has default value" triggers displaying default value control above the step controls, while step controls affect how default value picker works - that probably means that step controls should be above default value. <img alt="image" width="631" src="https://user-images.githubusercontent.com/15073128/161026010-8ac3b3d5-f271-44bb-9581-4bfc5233655e.png">
> > CC @kasiazjc
>
> That's an interesting observation but I have no idea how to do that. The default value stuff code is handled somewhere else, and honestly I'm not sure how it decides on the location of controlPanel components. In the controlPanel code for the Range Filter, I have
>
> ```
> {
> label: t('UI Configuration'),
> expanded: true,
> controlSetRows: [
> [
> {
> name: 'enableEmptyFilter',
> config: {
> type: 'CheckboxControl',
> label: t('Filter value is required'),
> default: false,
> renderTrigger: true,
> description: t(
> 'User must select a value before applying the filter',
> ),
> },
> },
> {
> name: 'enableSingleValue',
> config: {
> type: 'CheckboxControl',
> label: t('Single value'),
> default: SingleValueType.Exact,
> renderTrigger: true,
> description: t('Use only a single value.'),
> },
> },
> ],
> [
> {
> name: 'scaling',
> config: {
> type: 'SelectControl',
> label: t('Scaling Function'),
> default: PluginFilterRangeScalingFunctions.LINEAR,
> renderTrigger: true,
> freeForm: false,
> choices: Object.keys(
> SCALING_FUNCTION_ENUM_TO_SCALING_FUNCTION,
> ).map(key => [
> key,
> SCALING_FUNCTION_ENUM_TO_SCALING_FUNCTION[key].display,
> ]),
> description: t('Choose a scaling function for the slider.'),
> },
> },
> {
> name: 'stepSize',
> config: {
> type: 'SelectControl',
> label: t('Step Size'),
> default: 1,
> renderTrigger: true,
> freeForm: true,
> choices: [
> [0.001, 0.001],
> [0.01, 0.01],
> [0.1, 0.1],
> [1, 1],
> [2, 2],
> [10, 10],
> [25, 25],
> [100, 100],
> ],
> description: t('Set the slider step size.'),
> },
> },
> ],
> ],
> },
> ```
>
> But you can clearly see that's not the order the modal decides to put stuff. enableEmptyFilter is the last control in the modal, yet the first in the controlSetRows array. I still don't get exactly how that controlSetRows should be structured, as I would have thought the enableEmptyFilter and enableSingleValue controls would be on the same row (since they're in the same controlSetRow) which is (I believe) how it works for visualization controlPanel configs.
@stevetracvc @kgabryje
We had a conversation with @michael-s-molina who has suggested the following:
- The easiest solution is just to leave it in the current position if we can live with it (of course)
- Another way is to move the code directly into `FiltersConfigForm` between the description and the default value
You are right this wasn't straightforward forward but we aim to improve the codebase of native filters in the upcoming phase 2.
Some other observations that came out from our conversation:
- This functionality should be under a checkbox as it’s very specific, such as “Customise default step scaling function”
- The inputs should not take the full width of the Modal. Please follow the same UI of existing inputs
- If the step size is lower than two decimals, the exact number of decimals should also be visible in the left sidebar, otherwise it’s impossible to see those decimals changing.
Let me know if you have any questions. Thanks for the hard work!
--
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