andy-clapson commented on issue #26651:
URL: https://github.com/apache/superset/issues/26651#issuecomment-5716016069
@rusackas have a peek - any/all feedback good here. A bit verbose likely...
## Motivation
Opening the time filter on a chart with no time range set shows this:
```
Edit time range
Range type [No filter v]
--------------------------------
Actual time range
No filter
[CANCEL] [APPLY]
```
The only control is a dropdown. To pick a date you first have to know that
`Range type` is a mode selector, and that dates live behind `Last`,
`Previous`,
`Current`, `Custom` or `Advanced`.
It is the most common complaint from dashboard users. *"The number one
complaint I
get on my sites, by far"* (@rscarborough1996). *"Our users just want a
simple date
picker and their eyes glaze over"* (@treygilliland). Two teams have already
built
their own replacements (@luutomas / @JanZuska, @amaannawab923).
The thread already agrees on the fix. @webobite, @luutomas, @JanZuska,
@TasmiaZerin1128 and @yousoph's mockups all landed on the same thing: a
calendar
with preset shortcuts.
We assumed something must make that hard, and checked. Three assumptions, all
wrong:
- An antd `RangePicker` won't fit. It fits. The input is 282px, and the
calendar
panel portals to `document.body` so the popover doesn't bound it.
- The calendar panel gets clipped. antd positions it inside the viewport.
- Clicking a date dismisses the popover stack. It doesn't.
A working version is 207 added lines, no new dependencies, no migration. This
stalled for two years because nobody built it, not because it was hard.
## Proposed Change
### Part 1: a Calendar range type
Add `Calendar` to the existing control: quick range shortcuts plus an antd
`RangePicker`. Show it by default when a chart has no time filter.
Per @michael-s-molina and @geido, this uses the antd component rather than a
custom
picker. Superset is on `antd ^6.6.1`, so the upgrade that blocked this has
landed.
Kept small on purpose:
- Quick ranges emit values that already exist, covering the three families
@rusackas asked to keep represented: `Last day`/`Last week`/`Last month`
for
rolling windows, `previous calendar month` for the previous period, and
`Current month`/`Current year` for the current one. No new strings.
Note `Current month` spans the whole calendar month (`DATETRUNC(today,
MONTH)` to
the start of next month), so it is not month-to-date. The grammar has no
to-date form, and adding one would mean new strings.
- The picker emits the same `since : until` the parser already handles.
`GET /api/v1/time_range/` stays the only validator.
- Existing frames are untouched.
Out of scope, none of it needed to fix the default:
- Any change to the grammar or to stored values.
- Bound inclusivity. It is `since <= col < until` (`get_time_filter` in
`superset/models/helpers.py`), already shown in the `Actual time range`
readout.
- First day of week, weekend definitions, month and day naming. These come
from the
antd component's i18n.
- Fiscal calendars and week numbering. The grammar has no fiscal concept, so
that
is backend work and a separate proposal.
- Reorganising frames into tabs.
### Part 2: the extension path
@rusackas asked for this as an Extension contribution point. That is the
right end
state. One correction to the rationale, because it changes what gets built:
the
thread has not diverged on the default. @webobite, @luutomas, @JanZuska,
@TasmiaZerin1128 and @yousoph all drew the same shape, a calendar with preset
shortcuts, differing mainly in preset count and in whether the modes are
tabs or a
dropdown. Where deployments actually
differ is fiscal calendars, week numbering and domain-specific period logic,
and
core cannot ship every variant of those. That is where a contribution point
earns
its keep, and it is a stronger case than flexibility in general.
Most of the seam already exists:
- Every frame already implements `FrameComponentProps` (`value: string`,
`onChange: (timeRange: string) => void`), and the popover passes all
frames the
same props.
- Values already round-trip through `customTimeRangeEncode`/`Decode`,
exported from
`@superset-ui/core`.
- `guessFrame` maps a stored string to the frame that can edit it, falling
back to
`Advanced` when nothing matches, so an unclaimed value degrades to raw text
instead of stranding the user.
Missing: `FRAME_OPTIONS` is a `const` array rather than registry-derived, and
`guessFrame` is a hard-coded chain with no way for a contributed frame to
claim a
string.
Part 1 hits that second gap directly. A calendar-emitted range and a
hand-written
one are the same string, so any rule broad enough to reopen the picker's own
output
also claims pre-existing values. Frame ownership is already ambiguous in
core, and the
only lever available today is hand-tuning the claim, with no way for the
frames
themselves to declare what they can edit.
So a contribution point needs a claim mechanism, not just a render interface.
Roughly, and illustrative rather than offered for vote:
```ts
interface TimeRangeFrame {
id: string;
name: string;
canDecode(value: string): boolean;
Component: ComponentType<FrameComponentProps>;
}
```
The design question that mechanism raises is precedence. If contributed
frames are
checked before core ones, an extension can capture `Last week`. If they are
checked
after, a contributed frame can never replace `Custom`, which is what someone
replacing the picker actually wants. Answering that from a real
implementation is
worth more than answering it on paper.
Deliverables here: make `FRAME_OPTIONS` registry-derived, build one
reference frame
against it, and document the seam. The public contribution point follows in
its own
SIP once that reference frame has exercised the claim mechanism.
`Contributions` in
`@apache-superset/core` only declares a `sqllab` scope today, so publishing
one
needs new host architecture regardless.
## New or Changed Public Interfaces
A new `Calendar` option in the time range control. No grammar change, no
change to
stored values, no new REST surface. Part 2 adds no public API.
## New dependencies
None.
## Migration Plan and Compatibility
No data migration. Emitted values are unchanged.
Saved charts holding an absolute date range will open on `Calendar` rather
than
`Custom`/`Advanced`. The stored value is untouched. As above, this follows
from a
calendar-emitted range and a hand-written one being the same string. Ranges
carrying
a time component keep their time: the picker shows time fields when the
value has
one, so nothing is truncated.
Unfiltered charts open on `Calendar` instead of the empty `No filter` pane.
`No filter` is still selectable. `DEFAULT_TIME_FILTER` already controls the
default
value and is unaffected.
## Rejected Alternatives
- A custom picker. Maintenance and upgrade cost of diverging from antd.
- A time slider (Power BI style). Not precise enough. The Number range
filter can be
improved separately.
- A seventh frame alongside the existing six, as originally proposed in
2024. That
adds another choice to a control whose problem is too many choices. This
proposal
adds an option but also makes it the default.
- Reorganising into tabs, per @yousoph's mockups, which group the frames as
Absolute, Relative and Advanced. Probably the right end state and the
grouping
question is already answered, but it is a much larger change than fixing
the
default and is better proposed on its own.
- Using antd's built-in `presets` prop for the quick ranges. A preset's
value is a
concrete date tuple (`ValueDate<RangeValueType>` in rc-picker), so
selecting one
emits absolute dates. `Last week` would be saved frozen rather than as an
expression that re-resolves per query, and a saved dashboard would keep
showing
the same seven days. Superset's relative grammar cannot round-trip through
that
model, which is why the quick ranges sit outside the picker rather than
inside it.
This is the specific point where the component has to be augmented.
- Specifying a contribution point now. Scoped to the frontend it cannot
serve the
cases that actually differ between deployments (fiscal calendars, week
numbering),
because those need grammar support that does not exist.
## Open questions for [DISCUSS]
1. **Should the quick ranges be operator-configurable?** Proposed: yes. The
shipped
community implementations carry around nineteen presets, which is too
many for
most deployments and too few for some, and no fixed list will suit
everyone. A
config key following `DEFAULT_TIME_FILTER` (declared in
`superset/config.py`,
exposed through `FRONTEND_CONF_KEYS` in `superset/views/base.py`) would
take a
list of time-range strings:
```python
QUICK_TIME_RANGES = [
"Last day", "Last week", "Last month",
"previous calendar month", "Current month", "Current year",
]
```
Values only, with labels looked up from the existing option lists and
falling
back to the string itself. An empty list hides the row. This also gives
deployments a route to period shapes core does not ship, such as a genuine
month-to-date via `DATETRUNC(DATETIME('today'), MONTH) : now`, without
adding
grammar. Note the row is a single joined control and fits about six
entries at
the popover's 600px width, so longer lists need a different layout.
2. Does the default view need time-of-day precision, or does that stay in the
advanced frames?
3. Is the `ControlPopover` defect below a prerequisite, or tracked
separately?
4. Explore has no shared time range across temporal columns, so N columns
means N
ranges that can disagree. In scope or separate?
5. Continue the existing dev@ `[DISCUSS]` thread, or reopen?
## Appendix: defects found while measuring
Both pre-date this proposal and reproduce on unmodified `master`. Suggest
filing
separately.
- `ControlPopover.calculatePlacement` has a dead zone. A trigger with
`xRatio` and
`yRatio` both between 0.35 and 0.65 falls back to placement `left`, which
gets
flip-only overflow adjustment and cannot shift back into the viewport. So
whether
an oversized popover recovers depends on which row its trigger sits in and
on
window height. Predicted from source, confirmed in the DOM at two viewport
sizes.
- The antd 5 to 6 upgrade renamed `.ant-popover-inner` to
`.ant-popover-container`
and changed its padding from `12px` to `0 12px 12px`, losing 4px above the
popover
title across control popovers.
<img width="644" height="591" alt="Image"
src="https://github.com/user-attachments/assets/53ed4684-3de0-47a6-8ca6-2e61741a286f";
/>
<img width="633" height="602" alt="Image"
src="https://github.com/user-attachments/assets/8c30ce33-d7f9-4440-a453-4a2b63c46b2e";
/>
<img width="623" height="418" alt="Image"
src="https://github.com/user-attachments/assets/02b78cd3-2462-4848-8624-41b4f8c8854c";
/>
--
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]
