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

Comments

2 participants
@zaygraveyard
Contributor

zaygraveyard commented Jul 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jul 19, 2016

Owner

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?

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?

@gpbl gpbl added the new feature label Jul 19, 2016

@zaygraveyard

This comment has been minimized.

Show comment
Hide comment
@zaygraveyard

zaygraveyard Jul 19, 2016

Contributor

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

Contributor

zaygraveyard commented Jul 19, 2016

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

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jul 19, 2016

Owner

Would you send a PR? :)

Owner

gpbl commented Jul 19, 2016

Would you send a PR? :)

@zaygraveyard

This comment has been minimized.

Show comment
Hide comment
@zaygraveyard

zaygraveyard Jul 19, 2016

Contributor

Sure, I'll do that ASAP

Contributor

zaygraveyard commented Jul 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@zaygraveyard

zaygraveyard Jul 19, 2016

Contributor

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

What is the expected behavior?

Contributor

zaygraveyard commented Jul 19, 2016

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

What is the expected behavior?

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jul 19, 2016

Owner

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.

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

This comment has been minimized.

Show comment
Hide comment
@zaygraveyard

zaygraveyard Jul 19, 2016

Contributor

It's coming from the initialMonth prop.

Contributor

zaygraveyard commented Jul 19, 2016

It's coming from the initialMonth prop.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jul 19, 2016

Owner

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!

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

This comment has been minimized.

Show comment
Hide comment
@zaygraveyard

zaygraveyard Jul 19, 2016

Contributor

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?

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jul 19, 2016

Owner

@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!

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

This comment has been minimized.

Show comment
Hide comment
@zaygraveyard

zaygraveyard Jul 19, 2016

Contributor

Yeah sure 😄

Contributor

zaygraveyard commented Jul 19, 2016

Yeah sure 😄

@zaygraveyard

This comment has been minimized.

Show comment
Hide comment
@zaygraveyard

zaygraveyard Jul 19, 2016

Contributor

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.

Contributor

zaygraveyard commented Jul 19, 2016

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.

@gpbl gpbl closed this in 5196eb3 Jul 19, 2016

gpbl added a commit that referenced this issue Jul 19, 2016

Merge pull request #197 from zaygraveyard/feature/issue196
Add support for paged navigation. (fixes #196)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment