onefinestay / react-daterange-picker

Other
563 stars 208 forks source link

Allow updating Calendar props to reset the month and year #128

Closed jonniedarko closed 7 years ago

jonniedarko commented 8 years ago

This should deal with the issue #125 . if you want any changes to accept please let me know, I'd be happy to. styling on example page may be somewhere you might consider requiring an update

AlanFoster commented 8 years ago

@jonniedarko Leaving this PR for now works for me; Let me know when you're good to pick it up again :)

AlanFoster commented 8 years ago

@jonniedarko Would you mind adding some tests to this PR too please? :+1:

jonniedarko commented 8 years ago

@AlanFoster what would be a prefered way to approach setting the a highlighted range programmatically. Specifically, what do you feel should be the desired result if for example the start and end date are more than a month apart, e.g. if start was feb 1st and end was oct 1st? should each calendar display the start and end month? or just either the start/end and the month next to it?

jonniedarko commented 8 years ago

@AlanFoster Any chance of a review of the changes?

jonniedarko commented 8 years ago

@AlanFoster any chance of this being merged?

AlanFoster commented 8 years ago

@jonniedarko I've added some code changes as a PR, mind checking them out? https://github.com/jonniedarko/react-daterange-picker/pull/1 If you're happy we could get this merged in

Edit: And some clarifications on the questions above would be great! :+1:

eliseumds commented 8 years ago

Is the following behaviour expected?

http://g.recordit.co/iAeTqPcrWO.gif

Even though the newly selected range is visible, the calendar moves to the left.

jonniedarko commented 8 years ago

@eliseumds That behaviour is what I expected but maybe not ideal? @AlanFoster maybe hold of merging and I can try to address eliseumds feedback if you agree with it

eliseumds commented 8 years ago

@jonniedarko ye, it's a bit too much. I really like how this one behaves:

http://g.recordit.co/2TA4ZU1aOT.gif

jonniedarko commented 8 years ago

@eliseumds I have updated to take you feedback into account regards updating the displayed calendars. @AlanFoster I didn't address his other concern as I felt that should probably be a separate issue, I also updated the example you updated , just to make it a little more obvious to the user what kind of update can be made

jonniedarko commented 8 years ago

@AlanFoster FYI I updated the package json peer dependencies to be stricter about choosing react 14, you may wish to update to 15 but you will also need to update the react-addons-pure-render-mixin to 15 too, I choose not to update anything other than to make sure they were in sync

jonniedarko commented 8 years ago

@AlanFoster having issues dependences after rebasing of the master, tried updating dependences and tried setting them at react 0.14 but tests failing, can you have a look, i believe the current master branch on your repo is having the same issues

eliseumds commented 8 years ago

@jonniedarko wow, imo, experience improved a lot! Thank you.

AlanFoster commented 8 years ago

@jonniedarko Good catch; I've locked us down to React 0.14 I think we need more work in deciding whether the current year / month is visible. I was also thinking that we might want to jump to the end of the month for a moment range potentially?

jonniedarko commented 8 years ago

@AlanFoster What do you mean by jump to end of Month for range? Do you mean show two non consecutive months if Range is more than 2?

AlanFoster commented 8 years ago

@jonniedarko i.e. selecting a range which spans multiple months may cause this logic to break unexpectedly, and force it to snap to the very start of your selection again. This would be another variation of @eliseumds's GIF

eliseumds commented 8 years ago

@AlanFoster yeah, I guess this is actually how the date picker from Google Analytics behave, but it should only jump like that when there's a new range coming from props, not from user clicks.

AlanFoster commented 8 years ago

@jonniedarko @eliseumds I've put up a PR which is an extension of this work, #144 Would greatly appreciate a PR review if you guys get the chance :+1:

AlanFoster commented 7 years ago

@jonniedarko Thanks for the PR; This functionality should now be in master :+1:

jonniedarko commented 7 years ago

@AlanFoster nice to see it got in but it kinda sucks not to get the contribution having put the time and work into it. Definitely put me off contributing again!