Skip to content
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

Merged
merged 1 commit into from
Jul 13, 2018

Conversation

majapw
Copy link
Collaborator

@majapw majapw commented Jul 9, 2018

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

@majapw majapw force-pushed the maja-address-on-outside-click-errors branch from 3adae1b to 5a985eb Compare July 9, 2018 21:04
@majapw
Copy link
Collaborator Author

majapw commented Jul 9, 2018

@ljharb in fact, they do show up, but as warnings and not as errors? Is there any way to change that?

@coveralls
Copy link

coveralls commented Jul 9, 2018

Coverage Status

Coverage increased (+0.02%) to 84.453% when pulling 095864c on maja-address-on-outside-click-errors into 5f55b62 on master.

@@ -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';
Copy link
Member

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?

Copy link
Collaborator Author

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?

Copy link
Member

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

@majapw majapw force-pushed the maja-address-on-outside-click-errors branch from 5a985eb to e4f77fd Compare July 13, 2018 00:05
Copy link
Collaborator Author

@majapw majapw left a 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?

@majapw majapw force-pushed the maja-address-on-outside-click-errors branch from e4f77fd to 15a7542 Compare July 13, 2018 01:01
{input}
{this.maybeRenderDayPickerWithPortal()}
</OutsideClickHandler>
) : ([
Copy link
Member

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()}

Copy link
Collaborator Author

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

@majapw majapw force-pushed the maja-address-on-outside-click-errors branch 2 times, most recently from 97c03c2 to b6f4039 Compare July 13, 2018 17:18
@majapw majapw force-pushed the maja-address-on-outside-click-errors branch from b6f4039 to 095864c Compare July 13, 2018 17:25
@majapw majapw merged commit 2af9057 into master Jul 13, 2018
@majapw majapw deleted the maja-address-on-outside-click-errors branch July 13, 2018 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants