-
Notifications
You must be signed in to change notification settings - Fork 349
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(dropdown): add focus trap option that allows close on click #1126
Conversation
Pull Request Test Coverage Report for Build 3781
💛 - Coveralls |
PatternFly-React preview: https://1126-pr-patternfly-react-patternfly.surge.sh |
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.
Seems package-lock.json should not have changed?
Thanks for looking into this, @ibolton336! @kmcfaul Has a similar update in #1014. I posted feedback in that PR when I reviewed her updates using different interaction methods. Just to double-check, I also reviewed the preview for this PR. I'm getting the same results in both PRs for touch interaction:
|
Thanks @jgiardino - do you think it would make sense to open a separate issue for handling the mobile bug for the dropdown not closing when touching outside of the dropdown focus? |
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.
This fix is in PR #1014, but as it is undergoing edits for some unrelated fixes bundled with this one, I think this PR should be merged to get the fix out asap.
67feed2
to
7851087
Compare
7851087
to
aef16d5
Compare
@kmcfaul That sounds good to me. Also, @ibolton336 raised the question of whether the issue with handling touch events on mobile should be a separate issue. It's currently noted in the issue you're working on for PR #1014, but I'm curious if you have thoughts on separating that out. |
created #1131 to deal with the touch issues @jgiardino @kmcfaul |
What:
fixes #1113 and #1125
Additional issues: