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

Fixed unexpected blur call #1107

Merged
merged 1 commit into from
Apr 12, 2018
Merged

Conversation

marlonpp
Copy link
Contributor

@marlonpp marlonpp commented Apr 6, 2018

I'm using react-dates inside a Drop. This drop is triggered by an input. As of now, when I transition months the drop is getting closed unexpectedly. My drop closes on input blur, and react-dates calls activeElement.blur() after the month transition. I'm not entirely sure why this is there, but I believe it should only be done if the activeElement is a child of your container.

Hopefully this PR looks good to you, otherwise let me know what needs to be changed.

FYI. I added .vscode to .gitignore so that editor files are not accidentally added.

@coveralls
Copy link

coveralls commented Apr 7, 2018

Coverage Status

Coverage remained the same at 86.457% when pulling 8de8df0 on marlonpp:fix-unexpected-blur into d18da3b on airbnb:master.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems fine, but it does add a new dependency on “contains”; is there a way that this could use a modified https://github.com/airbnb/browser-shims/blob/master/document-contains.js that doesn’t modify the environment?

.gitignore Outdated
@@ -48,3 +48,5 @@ package-lock.json

css/styles.css

.vscode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this and add it it your personal global gitignore; individual editors’ files shouldn’t force a change in every repo that individual uses.

@marlonpp
Copy link
Contributor Author

marlonpp commented Apr 9, 2018

Hey Jordan,
Thanks for taking a look at it.
I could definitely use it, but don't you think this should be a concern at the application level?
Looking at the code, I see that contains is already being used here.
Also, contains seems to be a very well supported function (https://developer.mozilla.org/en-US/docs/Web/API/Node/contains).

@ljharb
Copy link
Member

ljharb commented Apr 10, 2018

Fair point :-) in that case, we've already got this issue and your PR doesn't add it, so let's go ahead with it!

I've rebased your branch for you.

@ljharb ljharb requested a review from majapw April 10, 2018 01:22
@ljharb ljharb added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Apr 10, 2018
Copy link
Collaborator

@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.

LGTM! :)

@majapw majapw merged commit dba1463 into react-dates:master Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch: fixes/refactors/etc Anything that's not major or minor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants