-
-
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
Do not pass in undefined onOutsideClick to the OutsideClickHandler #1256
Conversation
3adae1b
to
5a985eb
Compare
@ljharb in fact, they do show up, but as warnings and not as errors? Is there any way to change that? |
src/components/DateRangePicker.jsx
Outdated
@@ -529,7 +529,9 @@ class DateRangePicker extends React.Component { | |||
|
|||
const { isDateRangePickerInputFocused } = this.state; | |||
|
|||
const onOutsideClick = (!withPortal && !withFullScreenPortal) ? this.onOutsideClick : undefined; | |||
const enableOutsideClick = (!withPortal && !withFullScreenPortal); | |||
const Wrapper = enableOutsideClick ? OutsideClickHandler : 'div'; |
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.
it seems like we should be able to avoid adding the wrapping div entirely by only rendering the wrapper when enableOutsideClick
is true?
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 was thinking that I didn't want to change the DOM structure in this PR, but this is true. Do you think it could potentially cause problems if we removed this div?
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.
Hmm, I can't see why
5a985eb
to
e4f77fd
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 like to take another look?
e4f77fd
to
15a7542
Compare
src/components/SingleDatePicker.jsx
Outdated
{input} | ||
{this.maybeRenderDayPickerWithPortal()} | ||
</OutsideClickHandler> | ||
) : ([ |
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.
it seems like we could avoid the array, and the keys, if we did:
{enableOutsideClick && (
<OutsideClickHandler…
)}
{!enableOutsideClick && input}
{!enableOutsideClick && this.maybeRenderDayPickerWithPortal()}
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 was thinking about that but wasn't sure if that was preferable.
I guess making an array every time is p. gross. I'll update it
97c03c2
to
b6f4039
Compare
b6f4039
to
095864c
Compare
Fixes #1227
So I know we have #1239 open, but as an alternative, I wanted to propose this option instead as it wouldn't add the document event listeners at all (which seems preferable?).
As for regression tests, @ljharb, we render
withPortal
datepickers in tests, shouldn't the proptype error show up there?to: @ljharb