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

Add support for navigating with numberOfMonths #196

Closed
zaygraveyard opened this issue Jul 19, 2016 · 12 comments
Closed

Add support for navigating with numberOfMonths #196

zaygraveyard opened this issue Jul 19, 2016 · 12 comments

Comments

@zaygraveyard
Copy link
Contributor

I'm using this component with numberOfMonths = 4 and I need the navigation between months to be more like changing the page of months. E.g:

The visible months can be [1, 2, 3, 4] or [5, 6, 7, 8] or [9, 10, 11, 12]
So the visible months can never be [3, 4, 5, 6] for example.

I'm not setting the fromMonth and toMonth props so I can never run into the case where some of the months are not allowed.
But I understand that it can problematic.

One solution would be to only render the allowed months. E.g:
If the fromMonth = 2 and toMonth= 3 then we render [2, 3]
If the fromMonth = 2 and toMonth= 5 then we render [2, 3, 4], [5]

An other solution would be to say that the only change to the current behavior is that showNextMonth() and showPreviousMonth() will change the currentMonth by numberOfMonths instead of 1.

I would be happy to create a PR

@gpbl
Copy link
Owner

gpbl commented Jul 19, 2016

An other solution would be to say that the only change to the current behavior is that showNextMonth() and showPreviousMonth() will change the currentMonth by numberOfMonths instead of 1.

I like this idea. As default it should work as it does now (e.g. skipping only one month), but we could add a new prop to change the behavior of showNextMonth and showPreviousMonth. How could we call this new prop? pagedNavigation?

@zaygraveyard
Copy link
Contributor Author

Thank you for the fast reply.
A boolean pagedNavigation looks good to me.

@gpbl
Copy link
Owner

gpbl commented Jul 19, 2016

Would you send a PR? :)

@zaygraveyard
Copy link
Contributor Author

Sure, I'll do that ASAP

zaygraveyard added a commit to zaygraveyard/react-day-picker that referenced this issue Jul 19, 2016
zaygraveyard added a commit to zaygraveyard/react-day-picker that referenced this issue Jul 19, 2016
@zaygraveyard
Copy link
Contributor Author

I just noticed there's still this case:
numberOfMonths = 4
fromMonth = 3
currentMonth = 5
And we call showPreviousMonth()

What is the expected behavior?

@gpbl
Copy link
Owner

gpbl commented Jul 19, 2016

Actually the wrong value there is in currentMonth, that should never be 5. Is the currentMonth in this case coming from the initialMonth prop, or does it take this value after navigating the months? Need to check it out.

@zaygraveyard
Copy link
Contributor Author

It's coming from the initialMonth prop.

@gpbl
Copy link
Owner

gpbl commented Jul 19, 2016

Thanks, so we have two approach here:

  • the developer is responsible to check that initialMonth has the correct value
  • we add some logic in getStateFromProps to make currentMonth match the right month

We had a similar issue where [we preferred] the first option, but I'm open to change my mind about it!

@zaygraveyard
Copy link
Contributor Author

zaygraveyard commented Jul 19, 2016

I think it's the developer's responsibility, but we should also have a reasonable default behavior.
Something along the lines of:

getStateFromProps = props => {
  const initialMonth = Helpers.startOfMonth(props.initialMonth);
  let currentMonth = initialMonth;

  if (
    props.pagedNavigation    &&
    props.numberOfMonths > 1 &&
    props.fromMonth
  ) {
    const diffInMonths = Helpers.diffInMonths(currentMonth, fromMonth);

    currentMonth = Helpers.addMonths(
      fromMonth,
      Math.floor(diffInMonths / props.numberOfMonths) * props.numberOfMonths
    );
  }
  return {currentMonth};
}

What do you think? Do I add it to the PR?

@gpbl
Copy link
Owner

gpbl commented Jul 19, 2016

@zaygraveyard could you send the change above in an another PR? I'd prefer to deal with one issue at time (and we need tests for it as well) Thanks a lot!

@zaygraveyard
Copy link
Contributor Author

Yeah sure 😄

@zaygraveyard
Copy link
Contributor Author

The code for the second issue is ready, but it requires that #197 gets merged.
I'll send a PR as soon as the first one is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants