feat(explore): Clear temporal filter value

[kgabryje]

SUMMARY

Currently when there is 1 temporal filter added and user tries to remove it, we display a message saying that there must be at least 1 temporal filter in the chart. We use the same flow regardless if the filter has a value or not, so simply clearing the filter requires many additional clicks. This PR improves that behaviour:

• if there's more than 1 temporal filter, remove the filter on "x" click
• if there's 1 temporal filter and it has a value other than "No filter", set the value to "No filter" on "x" click
• if there's 1 empty temporal filter, display a warning message

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

https://github.com/apache/superset/assets/15073128/ba3897ab-3005-4e9e-9d9a-47e9ebd8e29c

TESTING INSTRUCTIONS

1. Go to explore and add 2 or more temporal filters with some values
2. Delete all but the last one
3. Try to delete the last one - instead of being removed, the filter should clear its value
4. Try to delete it again - a warning message should be displayed

ADDITIONAL INFORMATION

• [ ] Has associated issue:
• [ ] Required feature flags:
• [ ] Changes UI
• [ ] Includes DB Migration (follow approval process in SIP-59)
  • [ ] Migration is atomic, supports rollback & is backwards-compatible
  • [ ] Confirm DB migration upgrade and downgrade tested
  • [ ] Runtime estimates and downtime expectations provided
• [ ] Introduces new feature or API
• [ ] Removes existing feature or API

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 30.00000% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 69.83%. Comparing base (0d0e47a) to head (f127ef5).

Files	Patch %	Lines
.../src/explore/components/ControlPanelsContainer.tsx	12.50%	7 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #27788      +/-   ##
==========================================
- Coverage   69.83%   69.83%   -0.01%     
==========================================
  Files        1920     1920              
  Lines       75242    75248       +6     
  Branches     8423     8425       +2     
==========================================
+ Hits        52546    52550       +4     
- Misses      20635    20636       +1     
- Partials     2061     2062       +1     

Flag	Coverage Δ
javascript	57.42% <30.00%> (+<0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[kgabryje]

/testenv up

[github-actions[bot]]

@kgabryje Ephemeral environment spinning up at http://35.163.223.205:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[betodealmeida]

This seems a bit confusing... if we allow a temporal filter that says "No filter", why not allow having no filters? It seems like we're leaking an implementation detail to the user, where the API requires a filter (even if it says "No filter") so we require it in the UI.

It might be easier to allow having no filters, and simply sending "No filter" to the API if there are no filters, though ideally we'd fix this in the backend. Having the X do two different things — remove the filter or changing the filter time range depending on the number of filters — will confuse users.

cc: @yousoph

[michael-s-molina]

This seems a bit confusing... if we allow a temporal filter that says "No filter", why not allow having no filters? It seems like we're leaking an implementation detail to the user, where the API requires a filter (even if it says "No filter") so we require it in the UI.

@betodealmeida You're correct. It is confusing. The problem is that currently we need to know what's the temporal column used in the chart when applying dashboard native filters. That's why it always need at least one temporal filter configured. We discussed this problem many times internally and with @kasiazjc and we're going to write a SIP to completely change how this works by augmenting native filters to correctly handle time filters and remove this quirkiness.

[yousoph]

This seems a bit confusing... if we allow a temporal filter that says "No filter", why not allow having no filters? It seems like we're leaking an implementation detail to the user, where the API requires a filter (even if it says "No filter") so we require it in the UI.

Yeah, agreed with what @michael-s-molina mentioned that it's not quite ideal but we need to know what the temporal column is.

You're right that it is a little inconsistent when removing multiple filters but the idea was to remove a bit of friction from the user and reduce the frequency at which they see what feels like an error popup when the goal is to clear the filter values for now, until this flow is improved further.

[github-actions[bot]]

Ephemeral environment shutdown and build artifacts deleted.