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

Strange behavior when clicking days with outside modifier #97

Closed
JKillian opened this Issue Nov 20, 2015 · 5 comments

Comments

2 participants
@JKillian
Contributor

JKillian commented Nov 20, 2015

The first bug is that clicking outside days only changes the month if you have an onDayClick prop set (which is surprising behavior). This seems to be because handleDayClick is only called if props.onDayClick is set. I think this should maybe be opt-in behavior that happens regardless of onDayClick's value?

The second bug is that you can use this month changing functionality to navigate to days outside of the bounds set by fromMonth and toMonth. It seems that showMonthForOutsideDays doesn't check if the month is allowed or not. Perhaps instead of calling setState directly it should call showMonth instead?

@JKillian JKillian changed the title from Strange behavior when clicking outside day to Strange behavior when clicking days with outside modifier Nov 20, 2015

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Nov 21, 2015

Owner
  1. yes, this was a design choice
  2. this is indeed a bug 💧 i agree it should call showMonth and not directly set the state
Owner

gpbl commented Nov 21, 2015

  1. yes, this was a design choice
  2. this is indeed a bug 💧 i agree it should call showMonth and not directly set the state

@gpbl gpbl added the bug:minor label Nov 21, 2015

@gpbl gpbl closed this in d7aff64 Nov 24, 2015

gpbl added a commit that referenced this issue Nov 24, 2015

Merge pull request #104 from gpbl/issue-97
Do not show months for outside days (fix #97)
@JKillian

This comment has been minimized.

Show comment
Hide comment
@JKillian

JKillian Dec 3, 2015

Contributor

Thanks for the fix for the second bug! However, I think we might have messed up a little bit - instead of using showMonth, I think it should use showPreviousMonth and showNextMonth. This way, the onMonthChange prop will get called as it should be.

Contributor

JKillian commented Dec 3, 2015

Thanks for the fix for the second bug! However, I think we might have messed up a little bit - instead of using showMonth, I think it should use showPreviousMonth and showNextMonth. This way, the onMonthChange prop will get called as it should be.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Dec 3, 2015

Owner

@JKillian Our reasoning was right, because the old code is actually displaying a month based on the numberOfMonths prop, so not always is the next/prev month. However, this is actually an old behavior which should have already been changed. Going to fix this as well 👍

Owner

gpbl commented Dec 3, 2015

@JKillian Our reasoning was right, because the old code is actually displaying a month based on the numberOfMonths prop, so not always is the next/prev month. However, this is actually an old behavior which should have already been changed. Going to fix this as well 👍

@gpbl gpbl reopened this Dec 3, 2015

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Dec 3, 2015

Owner

See #112

Owner

gpbl commented Dec 3, 2015

See #112

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Dec 4, 2015

Owner

@JKillian these issues should be fixed in the new 1.2.0 release. There's event a new example showing the corrected behaviors.

Owner

gpbl commented Dec 4, 2015

@JKillian these issues should be fixed in the new 1.2.0 release. There's event a new example showing the corrected behaviors.

@gpbl gpbl closed this Dec 4, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment