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-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
