BugiDev / react-native-calendar-strip

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

Calling endOf('week') on the moment object in onWeekChanged callback is moving the calendar by 1 day #196

Closed rusticdeity closed 4 years ago

rusticdeity commented 4 years ago

When we try to navigate to the previous week the calendar strip is moving 1 day back instead of 1 week if the last method called on the moment object passed as part of onWeekChanged callback is .endOf('week').

Here is a snack to replicate this. I have put three methods of which two are commented and work as expected. However the third method isn't working as expected . I am assuming this might be an issue if the calendar strip is till using the moment object passed in as reference.

peacechen commented 4 years ago

Thanks for the Snack @GIT-Sachin

Did you mean to pass startDate state to CalendarStrip? Your code is using a locally created date.

      <CalendarStrip useIsoWeekday={false} startingDate={moment().startOf('week')}
                        ref={this.calendarRef} onDateSelected={this.resetAssignment} 
                        style={{ height: 90, paddingTop: 10, paddingBottom: 0 }}
                        onWeekChanged={this.handleWeekChange}
                    />

Note that Moment's startOf and endOf mutate the original dates. Clone the original date so you don't corrupt the values. https://momentjs.com/docs/#/manipulating/start-of/

Mutates the original moment by setting it to the start of a unit of time.

rusticdeity commented 4 years ago

@peacechen The reason of passing locally created date is to set the initial value of the calendar strip and let the calendar strip handle date changes. However, the moment object returned for onWeekChanged returns a moment object used internally by the calendar strip. I think it is better to return a cloned moment object and anyone who wants to control the calendar strip should do so via props rather than mutating the returned object.

peacechen commented 4 years ago

I agree that it would be better for CalendarStrip to return cloned dates to avoid the chance of users mutating a value used internally. Would you mind creating a PR? Does that solve this issue?

rusticdeity commented 4 years ago

@peacechen I am cloning the moment object returned from onWeekChanged and performing further actions. I'll create a PR and get back to you. This might take sometime since it is my first time contributing to an open source project.

peacechen commented 4 years ago

Thanks @GIT-Sachin

There are two relatively easy ways to develop on this library. One would be to modify the code in your project's node_modules/react-native-calendar-strip directory. You'd need to copy the code over to your fork of this repo after you get it working. The other way would be to adapt your code and place it into this project's example folder and test on the sample project.

rusticdeity commented 4 years ago

@peacechen I have created a pull request. Please have a look and let me know if i have to make any other changes.

peacechen commented 4 years ago

Published in 2.0.4. Thanks @GIT-Sachin for helping to identify and fix this issue.