-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Move the call to onDatesChange before the call to onFocusChange #1525
Move the call to onDatesChange before the call to onFocusChange #1525
Conversation
@majapw PTAL! |
If the ordering is important, then I think we should add a test that asserts it (also, you'll need to rebase) |
da543b2
to
957c028
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb would you say this is a breaking change? I'm worried about some edge-case of behavior being dependent on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I might feel better if we could add the tests to master, and then change them with this PR - that would clearly illustrate what was breaking or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the added tests look great
50e2e7d
to
95ee636
Compare
wrapper.instance().onDayClick(clickDate); | ||
expect(focusedInput).to.equal(END_DATE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does seem like a possible breaking change to me. @majapw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! I'm gonna release it as v19 after I release the previous changes in a minor.
This PR moves the call to
onDatesChange
beforeonFocusChange
. The purpose of this change is to allow the logic ofonDatesChange
to potentially affect the logic ofonFocusChange
.For example, I would like to allow users to click on certain unavailable dates and then show them a corresponding error state. When such a date is clicked, I do not want the focus to change because the date they clicked would not actually be selected. The logic of whether or not to change focus needs to occur in
onDatesChange
, so this function needs to be called beforeonFocusChange
.