BugiDev / react-native-calendar-strip

Easy to use and visually stunning calendar component for React Native.
MIT License
939 stars 327 forks source link

Selected date not highlighted if CalendarStrip.setSelectedDate is called inside onWeekChanged #134

Closed halileohalilei closed 4 years ago

halileohalilei commented 5 years ago

I want to use left and right selectors to change the selected date to a week before or after the current date. So, for example, if the week starts with 18 March and the selected date is the 20th, I want to have 13th selected if the user presses the left selector, and 27th if right.

I tried to implement this in the following way:

<CalendarStrip
  ref={(calendarStrip: CalendarStrip) => {
    this.calendarStrip = calendarStrip;
  }}
  onDateSelected={this.props.onDateSelected}
  onWeekChanged={(date: Date) => {
    const { calendarStrip } = this;
    const currentDate = calendarStrip.getSelectedDate();
    const currentDayOfWeek = getDay(currentDate);
    const newDate = addDays(date, (((currentDayOfWeek - 1) % 7) + 7) % 7); // handle modulo of negative numbers
    calendarStrip.setSelectedDate(newDate);
  }}
/>

This seems to update the week and the selected day as this.props.onDateSelected method correctly updates the state where I keep the current date. However, the selected date is not highlighted on the rendered view. I tried to trace what's going wrong and it seems calling CalendarStrip.setSelectedDate inside onWeekChanged causes the order of things, and results in a buggy view.

Am I doing anything wrong? Are there any other ways to achieve this?

peacechen commented 5 years ago

That may be a bug. Please try this change locally (inside node_modules/react-native-calendar-strip/). In the setSelectedDate() method at https://github.com/BugiDev/react-native-calendar-strip/blob/master/src/CalendarStrip.js#L420

change

    if (date !== 0) {
      this.updateWeekStart(mDate);
    }

to

    if (date !== 0) {
      this.setState({ startingDate: this.updateWeekStart(mDate) });
    }

If that works, please submit a PR.

halileohalilei commented 5 years ago

Tried it, but I still get the same behavior.

peacechen commented 5 years ago

What type of object do your getDay and addDays methods return?

halileohalilei commented 5 years ago

getDay returns a number and addDays returns a Date object.

peacechen commented 5 years ago

I re-reviewed the code and the changes I posted above aren't necessary. setSelectedDate should do the job. Set a breakpoint in setSelectedDate and confirm the the correct date is being passed in.

halileohalilei commented 5 years ago

@peacechen It's been a while but I've finally had a look at this again. I'm 100% sure that the date I'm passing is correct.

I played around with the code and believe that there's a race condition in CalendarStrip. I put a breakpoint in CalendarStrip.updateWeekData and it seems to be called multiple times when I use the code in my original post. Let's say that I'm on day 22.05.2019, so I expect the selected date to be 29.05.2019 when I click the right button. What happens is that updateWeekData is called once with the correct inputs, which are 27.05.2019 as startingDate and 29.05.2019 as selectedDate, and once again with incorrect inputs, which are 20.05.2019 as startingDate and 29.05.2019 as selectedDate.

One thing I found out was that if I change my code above from

<CalendarStrip
  ...
  onWeekChanged={(date: Date) => {
    ...
    calendarStrip.setSelectedDate(newDate);
  }}
/>

to

<CalendarStrip
  ...
  onWeekChanged={(date: Date) => {
    ...
    setTimeout(() => {
      calendarStrip.setSelectedDate(newDate);
    }, 10);
  }}
/>

then I get the behavior I expected in the first place. Needless to say, this is a very ugly and unreliable solution.

peacechen commented 5 years ago

Thanks for debugging this. It does sound like a race condition. In lieu of setTimeout, would you try runAfterInteractions and requestAnimationFrame ?

Documentation at https://facebook.github.io/react-native/docs/timers

halileohalilei commented 5 years ago

Alright, I was able to solve the problem without using any sort of timer. Instead of using the startingDate prop of the CalendarStrip, I used selectedDate and changed its value inside my onDateSelected callback. Now everything updates as I had intended to in the first place.

However, I did notice something that may be a bug in CalendarStrip.js. Following is the current implementation of setSelectedDate:

setSelectedDate(date) {
  let mDate = moment(date);
  this.onDateSelected(mDate);
  // Update week view only if date is not cleared (0).
  if (date !== 0) {
    this.updateWeekStart(mDate);
  }
}

As far as I understood, updateWeekStart returns a Date that is the starting day of the week mDate belongs to. However, this value is not used in the current implementation. Correct me if I'm wrong, but I think it should be something like this:

setSelectedDate(date) {
  let mDate = moment(date);
  this.onDateSelected(mDate);
  // Update week view only if date is not cleared (0).
  if (date !== 0) {
    const startingDate = this.updateWeekStart(mDate);
    const weekData = this.updateWeekData(startingDate, mDate, this.props);
    this.setState({ startingDate, ...weekData });
  }
}

Let me know if this isn't the place to discuss this and whether it needs a separate issue or a PR, or if there's nothing wrong with it at all. Cheers.

peacechen commented 5 years ago

Thanks again for looking into this. That does indeed look like a bug. The odd thing is it's working AFAIK, my first thought was to remove the call to updateWeekStart. However it's calling setLocale which is necessary for translations.

Would you mind validating that your fix works the same as it did before? We need to make sure it doesn't break any existing functionality. A PR would be great if you have time.

peacechen commented 4 years ago

Much of the date updating code has been refactored in 2.0.0. Closing this due to inactivity. Feel free to re-open or create a new issue if the bug persists.