CodeWithPravinMaske commented on issue #26651: URL: https://github.com/apache/superset/issues/26651#issuecomment-6358845239
@sfirke thanks, that plan makes sense and I have reworked #44527 to match it (your option 3). The branch is updated. **What changed** - No second filter type any more. The `filter_date_range` plugin, its constant and its preset registration are removed, along with the separate Alerts & Reports handling (reports already work with `filter_time`). - The existing **Time range** filter gets a new checkbox in its UI Configuration: **Calendar date picker**, off by default. Off is today's behaviour, so existing filters and dashboards are unchanged. - With it on, the filter bar shows only an inline date-range picker: no popover and no Range type dropdown. Picking dates gives a fixed range with the **end date included** (stored as end + 1 day, because the backend bound is exclusive). Clearing it means no filter. - The **default value** is still set with the full control, so a default such as "Last week" is possible. The filter settings dialog always uses the full control for the Default Value, and only the dashboard filter bar uses the calendar. In the calendar, an expression default is resolved by the backend (`/api/v1/time_range/`) and shown as its actual dates. - **Time-of-day edge case:** if a default carries a time of day (for example "10 days before now"), the picker shows the dates with a warning style and a tooltip: "This range includes a time of day, so it is not made of whole days. Pick dates to use whole days." I did not round, so the existing behaviour is kept. Happy to switch to rounding if you prefer. - The calendar is a **shared component**, `CalendarRangePicker`, next to the DateFilterControl frames (not inside a filter plugin), with the parse/format helpers in `DateFilterControl/utils/calendarRange.ts`. Part 1 can reuse both as they are, so the dashboard filter and Explore behave the same way. **Why a checkbox and not the Full / Calendar dropdown you sketched:** the filter settings dialog renders every UI Configuration option as a checkbox (only a few names such as `displayFormat` are special-cased), so a dropdown would need a special case there. I found this while testing locally: my first version used a dropdown and it appeared as a checkbox that saved `true`. I added a test so every Time-filter option stays a checkbox. If you would rather have the dropdown, I am happy to add that special case. **Tests:** new tests for the helpers, for `CalendarRangePicker` (fixed range, resolved expression, time-of-day warning, picking dates, clearing), for the filter plugin with the option on and off (including the settings dialog always using the full control), and for the settings option itself. The Time filter, DateFilterControl and native-filter suites pass, and lint and the type check are clean. **Screenshots** are attached below: the checkbox in the filter settings, the calendar in the filter bar, and the same filter with the option off. **Open question for you:** you suggested a config setting for the default of *new* filters. I left new filters with the checkbox off and kept this PR small. Do you want the config setting here, or as a follow-up PR together with Part 1? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
