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 2023/01/12 21:54:02 UTC
[GitHub] [superset] michael-s-molina opened a new pull request, #22707: feat: [WIP] Asks for confirmation when deleting the last temporal filter
michael-s-molina opened a new pull request, #22707:
URL: https://github.com/apache/superset/pull/22707
### SUMMARY
- Asks for confirmation when deleting the last temporal filter
- Adds an information message to Time Range native filter explaining the relationship with temporal columns
- Enables the save button when adding a new Time Range with No Filter value
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TODO
### TESTING INSTRUCTIONS
- Check that a modal requiring confirmation is presented when deleting the last temporal filter
- Check that Time Range native filters present a message describing it relationship with temporal columns
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] 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
- [ ] 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] michael-s-molina closed pull request #22707: feat: Generic X-axis improvements
Posted by GitBox <gi...@apache.org>.
michael-s-molina closed pull request #22707: feat: Generic X-axis improvements
URL: https://github.com/apache/superset/pull/22707
--
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] michael-s-molina closed pull request #22707: feat: Generic X-axis improvements
Posted by GitBox <gi...@apache.org>.
michael-s-molina closed pull request #22707: feat: Generic X-axis improvements
URL: https://github.com/apache/superset/pull/22707
--
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] michael-s-molina closed pull request #22707: feat: Generic X-axis improvements
Posted by GitBox <gi...@apache.org>.
michael-s-molina closed pull request #22707: feat: Generic X-axis improvements
URL: https://github.com/apache/superset/pull/22707
--
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] michael-s-molina merged pull request #22707: feat: Generic X-axis improvements
Posted by GitBox <gi...@apache.org>.
michael-s-molina merged PR #22707:
URL: https://github.com/apache/superset/pull/22707
--
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] michael-s-molina commented on a diff in pull request #22707: feat: Generic X-axis improvements
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #22707:
URL: https://github.com/apache/superset/pull/22707#discussion_r1073483260
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -258,6 +258,19 @@ const StyledAsterisk = styled.span`
}
`;
+const TimeRangeInfo = styled.div`
Review Comment:
👍🏼
--
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] sadpandajoe commented on pull request #22707: feat: Generic X-axis improvements
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #22707:
URL: https://github.com/apache/superset/pull/22707#issuecomment-1434924684
🏷️ preset:2023.3
--
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] michael-s-molina commented on a diff in pull request #22707: feat: Generic X-axis improvements
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #22707:
URL: https://github.com/apache/superset/pull/22707#discussion_r1072766755
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -431,6 +481,34 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
? baseDescription(exploreState, controls[name], chart)
: baseDescription;
+ if (name === 'adhoc_filters') {
+ restProps.confirmDeletion = {
+ triggerCondition: (
+ valueToBeDeleted: Record<string, any>,
+ values: Record<string, any>[],
+ ) => {
+ const isTemporalRange =
+ valueToBeDeleted.operator === Operators.TEMPORAL_RANGE;
+ if (isTemporalRange) {
+ const count = values.filter(
+ value => value.operator === Operators.TEMPORAL_RANGE,
+ ).length;
Review Comment:
Great suggestion! Done.
--
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 #22707: feat: [WIP] Asks for confirmation when deleting the last temporal filter
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22707:
URL: https://github.com/apache/superset/pull/22707#issuecomment-1381085926
# [Codecov](https://codecov.io/gh/apache/superset/pull/22707?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 [#22707](https://codecov.io/gh/apache/superset/pull/22707?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fc85785) into [master](https://codecov.io/gh/apache/superset/commit/5a422b31c5d1ee3059c7659b376e3b8e1eeb257b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a422b3) will **decrease** coverage by `0.01%`.
> The diff coverage is `26.31%`.
```diff
@@ Coverage Diff @@
## master #22707 +/- ##
==========================================
- Coverage 67.09% 67.07% -0.02%
==========================================
Files 1869 1869
Lines 71597 71614 +17
Branches 7821 7825 +4
==========================================
+ Hits 48035 48038 +3
- Misses 21534 21547 +13
- Partials 2028 2029 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `53.88% <26.31%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...Filters/FiltersConfigModal/FilterConfigurePane.tsx](https://codecov.io/gh/apache/superset/pull/22707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlckNvbmZpZ3VyZVBhbmUudHN4) | `100.00% <ø> (ø)` | |
| [.../FilterControl/AdhocFilterPopoverTrigger/index.tsx](https://codecov.io/gh/apache/superset/pull/22707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXJDb250cm9sL0FkaG9jRmlsdGVyUG9wb3ZlclRyaWdnZXIvaW5kZXgudHN4) | `83.33% <ø> (ø)` | |
| [...ontrols/DndColumnSelectControl/DndFilterSelect.tsx](https://codecov.io/gh/apache/superset/pull/22707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EbmRDb2x1bW5TZWxlY3RDb250cm9sL0RuZEZpbHRlclNlbGVjdC50c3g=) | `39.53% <20.00%> (-3.08%)` | :arrow_down: |
| [...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx](https://codecov.io/gh/apache/superset/pull/22707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL0ZpbHRlcnNDb25maWdGb3JtLnRzeA==) | `59.23% <33.33%> (-0.28%)` | :arrow_down: |
| [...ols/FilterControl/AdhocFilterEditPopover/index.jsx](https://codecov.io/gh/apache/superset/pull/22707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXJDb250cm9sL0FkaG9jRmlsdGVyRWRpdFBvcG92ZXIvaW5kZXguanN4) | `70.21% <100.00%> (ø)` | |
| [superset-frontend/src/views/routes.tsx](https://codecov.io/gh/apache/superset/pull/22707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL3JvdXRlcy50c3g=) | `55.00% <0.00%> (ø)` | |
| [...perset-frontend/src/views/CRUD/chart/ChartCard.tsx](https://codecov.io/gh/apache/superset/pull/22707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvY2hhcnQvQ2hhcnRDYXJkLnRzeA==) | | |
| [...perset-frontend/src/addSlice/AddSliceContainer.tsx](https://codecov.io/gh/apache/superset/pull/22707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2FkZFNsaWNlL0FkZFNsaWNlQ29udGFpbmVyLnRzeA==) | | |
| [...perset-frontend/src/views/CRUD/chart/ChartList.tsx](https://codecov.io/gh/apache/superset/pull/22707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvY2hhcnQvQ2hhcnRMaXN0LnRzeA==) | | |
| [...uperset-frontend/src/pages/ChartCreation/index.tsx](https://codecov.io/gh/apache/superset/pull/22707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3BhZ2VzL0NoYXJ0Q3JlYXRpb24vaW5kZXgudHN4) | `61.11% <0.00%> (ø)` | |
| ... and [2 more](https://codecov.io/gh/apache/superset/pull/22707?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #22707: feat: Asks for confirmation when deleting the last temporal filter
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #22707:
URL: https://github.com/apache/superset/pull/22707#issuecomment-1382937249
I'll handle the dialog buttons theming in a separate PR as it's related to the `Modal` component.
--
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] michael-s-molina commented on pull request #22707: feat: Generic X-axis improvements
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #22707:
URL: https://github.com/apache/superset/pull/22707#issuecomment-1386006369
> Found a bug while testing: It seems the filter modal comment does not show up when adding the time range, but only after saving the filters and opening the filter bar modal again. Also, changing the filter type to something else doesn't remove it (same issue I assume).
Fixed.
--
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] villebro commented on a diff in pull request #22707: feat: Generic X-axis improvements
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #22707:
URL: https://github.com/apache/superset/pull/22707#discussion_r1073203859
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -258,6 +258,19 @@ const StyledAsterisk = styled.span`
}
`;
+const TimeRangeInfo = styled.div`
Review Comment:
As I assume this can function as a generic filter type info component, this could potentially be renamed to `FilterTypeInfo`
--
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] villebro commented on a diff in pull request #22707: feat: Generic X-axis improvements
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #22707:
URL: https://github.com/apache/superset/pull/22707#discussion_r1071859874
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -431,6 +481,34 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
? baseDescription(exploreState, controls[name], chart)
: baseDescription;
+ if (name === 'adhoc_filters') {
+ restProps.confirmDeletion = {
+ triggerCondition: (
+ valueToBeDeleted: Record<string, any>,
+ values: Record<string, any>[],
+ ) => {
+ const isTemporalRange =
+ valueToBeDeleted.operator === Operators.TEMPORAL_RANGE;
+ if (isTemporalRange) {
+ const count = values.filter(
+ value => value.operator === Operators.TEMPORAL_RANGE,
+ ).length;
+ if (count < 2) {
+ return true;
+ }
+ }
+ return false;
+ },
+ confirmationTitle: t(
+ 'Are you sure you want to remove the last temporal filter?',
+ ),
+ confirmationText: t(
+ `This filter is the last temporal filter. If you proceed,
+ your charts won't be affected by time range filters in dashboards.`,
Review Comment:
as we can only do this for one chart at a time, this seems more appropriate:
```suggestion
`This filter is the last temporal filter. If you proceed,
this chart won't be affected by time range filters in dashboards.`,
```
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -431,6 +481,34 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
? baseDescription(exploreState, controls[name], chart)
: baseDescription;
+ if (name === 'adhoc_filters') {
+ restProps.confirmDeletion = {
+ triggerCondition: (
+ valueToBeDeleted: Record<string, any>,
+ values: Record<string, any>[],
+ ) => {
+ const isTemporalRange =
+ valueToBeDeleted.operator === Operators.TEMPORAL_RANGE;
+ if (isTemporalRange) {
+ const count = values.filter(
+ value => value.operator === Operators.TEMPORAL_RANGE,
+ ).length;
Review Comment:
Is declaring `isTemporalRange` really necessary? Just having this in the if statement seems legible enough. Alternatively, if we want to DRY checking for `TEMPORAL_RANGE`, maybe we could just have a simple func for it that we can use in both the if and `values.filter()`:
```js
const isTemporalRange(flt) => flt === Operators.TEMPORAL_RANGE;
if (isTemporalRange(valueToBeDeleted)) {}
const count = values.filter(isTemporalRange);
...
```
--
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