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

Fix date selection issue with withPortal or appendToBody. Fixes #1535. #1540

Merged
merged 4 commits into from Feb 12, 2019

Conversation

Projects
None yet
5 participants
@danedavid
Copy link

danedavid commented Feb 10, 2019

Fixes #1535 .

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 10, 2019

Coverage Status

Coverage remained the same at 84.504% when pulling 0351ccb on danedavid:issues/issue-1535 into 7c287db on airbnb:master.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Feb 10, 2019

This appears to be failing tests.

@danedavid

This comment has been minimized.

Copy link
Author

danedavid commented Feb 10, 2019

@ljharb Ok, let me check and get back to you

@danedavid

This comment has been minimized.

Copy link
Author

danedavid commented Feb 10, 2019

@ljharb Updated the PR.

if (this.container.contains(e.relatedTarget || e.target)) return;
if (
this.dayPickerContainer
&& this.dayPickerContainer.contains(e.relatedTarget || e.target)

This comment has been minimized.

@ljharb

ljharb Feb 10, 2019

Member

I’m not sure this is the right change - is it possible the relevant tests were mocking this.container but need to be updated to mock this.dayPickerContainer?

This comment has been minimized.

@danedavid

danedavid Feb 10, 2019

Author

Even I thought of that. The fact is that there are always 2 refs - the outer ref this.container, and the inner ref this.dayPickerContainer. The issue was caused because the wrong container ref was being searched in case of elements rendered in portal. So I referred to DateRangePicker.jsx, where I saw that this.dayPickerContainer was the one being used in focusOut handler. And thus the change.
So should we update the tests, or is the change wrong?

This comment has been minimized.

@ljharb

ljharb Feb 10, 2019

Member

Your change might be perfect :-) but maybe you could add tests that would help ensure it's not broken in the future?

This comment has been minimized.

@danedavid

danedavid Feb 11, 2019

Author

I've changed the ref being used inside the test, and reverted the previous check. @ljharb

@majapw

majapw approved these changes Feb 11, 2019

Copy link
Contributor

majapw left a comment

Seems reasonable to me. cc: @monokrome it seems like your fix was incomplete.

@danedavid Does the DateRangePicker still currently work?

@ljharb

ljharb approved these changes Feb 11, 2019

@majapw majapw merged commit 9575669 into airbnb:master Feb 12, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 84.504%
Details
@monokrome

This comment has been minimized.

Copy link
Contributor

monokrome commented Feb 12, 2019

@majapw Not sure that my fix didn't work. Seems unrelated to me - which use case wasn't working in 19.0.2?

@majapw

This comment has been minimized.

Copy link
Contributor

majapw commented Feb 12, 2019

@monokrome Sorry I wasn't clear! This issue seemed very similar to #1522 (clicking anywhere closes the SingleDatePicker) as it was related to the SDP autoclosing and the appendToBody prop. This seemed like a specific edgecase of that (the description of the use case is in the linked issue) and both were rooted in the v18.4.1 changes. Thanks for taking a look!

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