[pickers] DateRangePicker's `onAccept` callback is not called for same dates

MartinJaskullaTS commented 1 month ago

The DateRangePicker's onAccept callback is not called if the same date range is selected. This is by design.

However, this design does not allow some use cases.

I am using shortcuts, which also trigger onAccept when clicked. There can be semantically different shortcuts that result in the same date range in certain situations. These are unusable at the moment.

Let's say I have 2 shortcuts:

Christmas 2023
Christmas last year

If its 2024, both would set the range 24.12.2023 - 24.12.2023. If its 2025, one would set 24.12.2023 - 24.12.2023, the other 24.12.2024 - 24.12.2024.

If a shortcut is clicked, I add it as a query parameter to the URL e.g. &preset=christmas_23 or &preset=christmas_last_year. This way users can share the link or bookmark it.

Let's say its 2024 and a user clicks "Christmas 2023". Now they want to change to "Christmas last year", bookmark it and open the bookmark in 2025, 2026 etc. They can't, because onAccept is not called. There is also no convenient way for me to get notified about the click, because each individual shortcut does not have an onClick property.

I think onAccept should always be called. The feature that MUI is checking if the dates did not change to not call onAccept does not provide a lot of value. Checking this myself in my onAccept callback is trivial. If always calling onAccept is not possible, then at least there should be a convenient to know, which shortcut was clicked.

The Christmas example is somewhat contrived. My actual use case is a date range comparison feature in an analytics dashboard, where I have two date range inputs, which both have shortcuts. Some shortcuts of the second comparison range can in certain scenarios have same date range. This depends on what date range was selected in the first base date range.

Combination 1: Input 1 shortcut: Last 365 days -> 26.09.23 - 24.09.23 Input 2 shortcut: Previous year (same period) -> 26.09.23 - 25.09.23

Combination 2: Input 1 shortcut: Last 365 days -> 26.09.23 - 24.09.23 Input 2 shortcut: Preceding period -> 26.09.23 - 25.09.23

"Previous year (same period)" is currently selected in input 2. The user is not able to click on "Preceding period" via the shortcut (red rectangle). They might want to switch the comparison "mode" (the shortcut) first and then change input 1 to "Last 7 days". If they do that, the two shortcuts ("Previous year...", "Preceding period", ) would result in different date ranges.

Your environment

`npx @mui/envinfo`

``` System: OS: macOS 14.6.1 Binaries: Node: 16.20.2 - ~/.nvm/versions/node/v16.20.2/bin/node npm: 8.19.4 - ~/.nvm/versions/node/v16.20.2/bin/npm pnpm: Not Found Browsers: Chrome: 128.0.6613.120 Edge: Not Found Safari: 17.6 npmPackages: @emotion/react: 11.13.3 @emotion/styled: 11.13.0 @mui/core-downloads-tracker: 6.1.1 @mui/icons-material: 6.1.1 @mui/material: 6.1.1 @mui/private-theming: 6.1.1 @mui/styled-engine: 6.1.1 @mui/system: 6.1.1 @mui/types: 7.2.17 @mui/utils: 6.1.1 @mui/x-date-pickers: 7.18.0 @mui/x-date-pickers-pro: 7.18.0 @mui/x-internals: 7.18.0 @mui/x-license: 7.18.0 @types/react: 18.3.3 => 18.3.3 react: 18.3.1 react-dom: 18.3.1 typescript: 5.5.3 => 5.5.3 ```

Search keywords: daterangepicker shortcuts onaccept Order ID: 96659

MartinJaskullaTS commented 1 month ago

I just noticed that the same issue exists when selecting a custom date range e.g. clicking on today's date in the calendar, which makes a "Today" shortcut also unusable, which would add it as a query param to allow bookmarking/sharing.

MartinJaskullaTS commented 1 month ago

This shows a workaround I tried and another behaviour I don't understand: https://codesandbox.io/p/sandbox/dx8gs6

Why does the DateRangePicker become empty after setting value to the current date range?

function B() {
  const [value, setValue] = useState<[Dayjs | null, Dayjs | null]>([
    dayjs("01-01-2020"),
    dayjs("01-01-2020"),
  ]);

  return (
    <>
      <p>onAccept called, but input becomes empty, then broken:</p>
      <DateRangePicker
        slotProps={{
          shortcuts: {
            items: [
              {
                label: "01.01.2020",
                getValue: () => {
                  // Fake date that differs from current value to trigger onAccept
                  return [null, null];
                },
              },
            ],
          },
        }}
        value={value}
        onAccept={(v, context) => {
          alert("onAccept called");
          // Set the real shortcut value. However, DateRangePicker breaks, because the same dates are set.
          setValue([dayjs("01-01-2020"), dayjs("01-01-2020")]);
        }}
      />
    </>
  );
}

I guess that DateRangePicker has an internal check that compares the new value prop to the previous one and then something unexpected happens. But why have such a check? If I pass in a value, I expect it to be shown.

MartinJaskullaTS commented 1 month ago

Found a better workaround, which I still don't like.

shortcuts: {
            onClick: (e) => console.log(e.target),
            items: [
              {
                id: "1",
                label: "01.01.2020",
                getValue: () => {
                  return [dayjs("01-01-2020"), dayjs("01-01-2020")];
                },
              },
            ],
          },

e.target will be either ul, li, <div id="1"> or <span>01.01.2020</span> depending on where you click on the shortcut. Based on the id, I can find the clicked shortcut.

Sidenote: Maybe it would be better for <div id="1"> to use a data attribute: <div data-shortcut-id="1"> instead to prevent multiple elements with the same id.

michelengelen commented 1 month ago

I get what you are trying to achieve and I do think that both approaches actually make sense. We could introduce a prop to allow for a commit even when the values did not change. From https://github.com/mui/mui-x/blob/0f0a7ec0ea4ffcd09e1290919ae3a1b714d74746/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts#L261

- hasChanged: (comparison) => !valueManager.areValuesEqual(utils, action.value, comparison),
+ hasChanged: (comparison) => rootProps.commitEqualValues || !valueManager.areValuesEqual(utils, action.value, comparison),

@LukasTy WDYT?

michelengelen commented 1 month ago

I get what you are trying to achieve and I do think that both approaches actually make sense. We could introduce a prop to allow for a commit even when the values did not change. From

https://github.com/mui/mui-x/blob/0f0a7ec0ea4ffcd09e1290919ae3a1b714d74746/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts#L261
- hasChanged: (comparison) => !valueManager.areValuesEqual(utils, action.value, comparison),
+ hasChanged: (comparison) => rootProps.commitEqualValues || !valueManager.areValuesEqual(utils, action.value, comparison),
@LukasTy WDYT?

On second though the name might be misleading and adding it in the hasChanged is semantically incorrect. Better solution:

- const shouldCommit = shouldCommitValue(updaterParams);
+ const shouldCommit = rootProps.commitEqualValues || shouldCommitValue(updaterParams);

LukasTy commented 1 month ago

I just noticed that the same issue exists when selecting a custom date range e.g. clicking on today's date in the calendar, which makes a "Today" shortcut also unusable, which would add it as a query param to allow bookmarking/sharing.

[...]

I guess that DateRangePicker has an internal check that compares the new value prop to the previous one and then something unexpected happens. But why have such a check? If I pass in a value, I expect it to be shown.

In this regard, you seem to be bumping into this issue: https://github.com/mui/mui-x/issues/10424 🙈

In regards to onAccept being called after every change, regardless of the value change: I would like us to avoid adding props here to control the behavior. 🙈 IMHO, either we remove the logic comparing the current value with the last published one and call onAccept every time or add the option to control this behavior externally (hook or some other means). @flaviendelangle, you have worked quite a bit on the lifecycle. WDYT, would ditching the comparison and calling onAccept more often be feasible? Or are there any known roadblocks? 🤔

MartinJaskullaTS commented 1 month ago

If you don't change anything about onAccept and instead allow passing every prop of Chip to each shortcut, you solve other problems as well e.g. shortcuts with long text having text-overflow: ellipsis, but no way of showing a title on hover.

So maybe:

<DateRangePicker
        slotProps={{
          shortcuts: {
            items: [
              {
                getValue: () => {...},
                label: "Very loooooooooooooooooooooooooong label",
                // Allow all properties of Chip (each shortcut is a Chip)
                onClick: () => {} // Fixes the problem of this issue
                title: "Very loooooooooooooooooooooooooong label" // Fixes the text overflow issue
              },
            ],
          },
        }}
      />

Or

<DateRangePicker
        slotProps={{
          shortcuts: {
            items: [
              {
                getValue: () => {...},
                slotProps: {
                  label: "Very loooooooooooooooooooooooooong label",
                  onClick: () => {}
                  title: "Very loooooooooooooooooooooooooong label"
                }
              },
            ],
          },
        }}
      />

You can see the text overflow issue also in the screenshot here: "Same period last year (...".

Just to add title I need to provide my own custom shortcuts slot.

mui / mui-x

[pickers] DateRangePicker's `onAccept` callback is not called for same dates #14717

Your environment