Skip to content

Conversation

@shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Aug 6, 2019

This is an attempted fix for some weird issues I encountered in primer/primer.style#166: Namely, that the event.preventDefault() call inside our Details menu close handler is preventing the browser from actually following links in a dropdown.

The proposed solution is to only close the menu if the target of the click event's closest <details> element is not the one being rendered. In other words, we only close the menu if the user clicks outside of the rendered element. Even in that case, I think we wouldn't want to call event.preventDefault() since that will prevent clicks on elements like links from working the first time.

@shawnbot shawnbot requested review from colebemis and emplums August 6, 2019 22:35
@emplums
Copy link

emplums commented Aug 7, 2019

Hey! I believe this is solved in #499. That being said, 14.0.0 isn't meant to be released for a while. I'm a little worried about side effects that might occur from this change since we ran into so many edge cases while working through #499.
Some of the things to test are:

  • Dropdown behaves properly
  • When there are more than one Details or Dropdown elements on the page, everything interacts as expected
  • The overlay prop works as expected
  • All the different ways to render children still work as expected

I found that writing tests to that worked without false positives/negative was different because it seems like the event delegation in Enzyme works a bit differently than it would in the browser, so some events were firing out of order and causing weirdness that I couldn't replicate outside of the test environment.

Are you thinking of this PR as a bandaid until 14.0.0 comes out, or a replacement for the work in 14.0.0?

@shawnbot
Copy link
Contributor Author

shawnbot commented Aug 7, 2019

Are you thinking of this PR as a bandaid until 14.0.0 comes out, or a replacement for the work in 14.0.0?

I think this is the right way to do it, but I'm nowhere near 100% sure. I think it merits some investigation, and I support you in moving forward with the changes in #499 for 14.0.0.

@emplums emplums changed the base branch from master to release-14.0.0 September 24, 2019 21:41
@emplums emplums marked this pull request as ready for review September 24, 2019 21:41
@vercel vercel bot temporarily deployed to staging September 24, 2019 21:44 Inactive
@vercel
Copy link

vercel bot commented Sep 24, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://primer-components-git-details-event-fix.primer.now.sh

@vercel vercel bot temporarily deployed to staging September 24, 2019 21:47 Inactive
@vercel vercel bot temporarily deployed to staging September 24, 2019 21:50 Inactive
@vercel vercel bot temporarily deployed to staging September 24, 2019 21:50 Inactive
Copy link

@emplums emplums left a comment

Choose a reason for hiding this comment

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

Going to go ahead and merge this into the tracking branch finally! 🎉

@vercel vercel bot temporarily deployed to staging September 24, 2019 21:57 Inactive
@emplums emplums merged commit d70a101 into release-14.0.0 Sep 24, 2019
@emplums emplums deleted the details-event-fix branch September 24, 2019 22:00
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