Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Range mode has its own internal state and does not respect the controlled state #2362

Closed
matheusmb opened this issue Aug 9, 2024 · 4 comments · Fixed by #2462
Closed

Range mode has its own internal state and does not respect the controlled state #2362

matheusmb opened this issue Aug 9, 2024 · 4 comments · Fixed by #2462
Assignees
Labels
bug Bug or Bug fixes in progress Work in Progress

Comments

@matheusmb
Copy link

Code

import { useState } from 'react';

import { DateRange, DayPicker } from 'react-day-picker';

export function ControlledCalendar() {
  const [selected, setSelected] = useState<DateRange | undefined>();

  return (
    <DayPicker
      mode="range"
      selected={selected}
      onSelect={(selected) => {
        // setSelected(selected);
      }}
    />
  );
}

Expected Behavior

Since the component is being used in "controlled" mode, it should use the state passed via "selected" prop. In the example above, the selected state is purposely not updated. It was expected that the calendar would not work, since the selected values aren't being updated.

I was trying to implement a behavior where if the range is already selected, and the user click on a new day, the range is reset:

import { useState } from 'react';

import { DateRange, DayPicker } from 'react-day-picker';

export function ControlledCalendar() {
  const [selected, setSelected] = useState<DateRange | undefined>();

  function handleOnSelect(range: DateRange, triggerDate: Date) {
    if (selected?.from && selected?.to) {
      setSelected({
        from: triggerDate,
        to: undefined,
      });
      return;
    }
    setSelected(selected);
  }

  return <DayPicker mode="range" required selected={selected} onSelect={handleOnSelect} />;
}

Since the internal state is updated via useEffect, it causes a small flicker. The sequence of events is:
User click -> internal range state updated -> onSelect callback -> external state updated (range reset) -> internal state is updated via useEffect due external state change.

Actual Behavior

The calendar has its own internal state. The external state is updated via useEffect.

CodeSandbox

Video

Tab-App.tsx.nodebox.CodeSandbox.mp4

PS: I throttled the CPU in 6x to make it more noticeable

@gpbl gpbl added bug Bug or Bug fixes to investigate Needs more investigation from the mantainers labels Aug 17, 2024
@gpbl
Copy link
Owner

gpbl commented Aug 17, 2024

thanks @matheusmb for your detailed report ! you are right the controlled vs uncontrolled state isn't the ideal here.

The code should read something like:

const handleSelect = (value: string) => {
    if (onSelect) {
      onSelect(value); // If the component is controlled, trigger the onSelect callback
    } else {
      setInternalSelected(value); // If the component is uncontrolled, update the internal state
    }
  };

@gpbl gpbl removed the to investigate Needs more investigation from the mantainers label Aug 17, 2024
@matheusmb
Copy link
Author

matheusmb commented Aug 18, 2024

Hey @gpbl,sure thing! I like the way Chakra-UI implemented it, it might be useful to take a look at:

@gpbl
Copy link
Owner

gpbl commented Sep 7, 2024

Hey this is more difficult than I thought. Please have patience...

@gpbl
Copy link
Owner

gpbl commented Sep 7, 2024

@matheusmb to help - could you create a (failing) test case, reproducing the issue? 🙏🏽

@gpbl gpbl added the PR Welcome Welcome to Pull Request label Sep 7, 2024
@gpbl gpbl self-assigned this Sep 15, 2024
@gpbl gpbl added in progress Work in Progress and removed PR Welcome Welcome to Pull Request labels Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug or Bug fixes in progress Work in Progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants