Added OutsideClickHandler component with onOutsideClick callback#666
Added OutsideClickHandler component with onOutsideClick callback#666ljharb merged 4 commits intoreact-dates:masterfrom
Conversation
| isRTL={isRTL} | ||
| /> | ||
| <OutsideClickHandler | ||
| onOutsideClick={onOutsideClick} |
There was a problem hiding this comment.
if onOutsideClick is omitted, should the DayPicker still be wrapped?
There was a problem hiding this comment.
I do not see any negative impact if onOutsideClick is omitted. Are you thinking to conditionally wrap the DayPicker component depending on the onOutsideClick value?
There was a problem hiding this comment.
Yes, that way if people don't want outside click behavior, they wouldn't incur any overhead.
|
@ljharb - Updated PR with suggestion. |
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
Thanks - I was thinking more, not wrapping it at all rather than wrapping it in a div - you could store the DayPicker element in a variable, and then repeat that in both if/else code paths.
|
|
||
| const { currentMonth, visibleDays } = this.state; | ||
|
|
||
| const DayPickerWrapper = (onOutsideClick) ? OutsideClickHandler : 'div'; |
There was a problem hiding this comment.
Let's remove these unnecessary parents on the ternary conditionals.
| const { currentMonth, visibleDays } = this.state; | ||
|
|
||
| const DayPickerWrapper = (onOutsideClick) ? OutsideClickHandler : 'div'; | ||
| const wrapperProps = (onOutsideClick) ? { onOutsideClick } : {}; |
There was a problem hiding this comment.
This shouldn't be needed; onOutsideClick={null} on a DOM element is a no-op.
|
@ljharb - Thanks for the feedback, updated PR. |
|
I'm interested in why the refactor failed. I too think I would prefer what @ljharb suggested and don't know why that would cause tests to fail. |
|
@majapw - all of the DayPickerSingleDateController tests were getting the following error: |
|
@mattRoyten You had an error in your code, in the unwrapped case, you were returning an object (ie, Could you rebase on the command line so as to remove all merge commits, and also remove the last revert commit, and fix that error? |
mattRoyten
force-pushed
the
outsideClick
branch
from
2e6e9b3 to
24662b8
Compare
|
@ljharb - got it. Updated. |
| isRTL={isRTL} | ||
| /> | ||
| ); | ||
| const dayPickerComponent = (<DayPicker |
There was a problem hiding this comment.
one last nit: the ( should end the line, and the ); should begin one - iow:
const foo = (
<div>
</div>
);
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
Looks like we need some test coverage of this - a single shallow-render should do it :-)
majapw
left a comment
majapw
left a comment
There was a problem hiding this comment.
Let's add a test! :)
(one that checks for the presence of the OutsideClickHandler when that's passed in and for its absence when it's not should be sufficient)
| /> | ||
| ); | ||
|
|
||
| if (onOutsideClick) { |
There was a problem hiding this comment.
Oh this looks so much better
|
@majapw - tests added! |
| onOutsideClick={() => null} | ||
| />, | ||
| ); | ||
| expect(wrapper.find(OutsideClickHandler).length).to.equal(1); |
There was a problem hiding this comment.
expect(wrapper.find(OutsideClickHandler).to.have.lengthOf(1) has a nicer failure message; nbd tho :-)
There was a problem hiding this comment.
sure, want me to change this as well?
5 participants
Corrected onOutsideClick functionality missing from the DayPickerSingleDateController component. This prop was within the defaultProps and now is properly applied.