fix(menus): Lazy attach auto close handlers #149
Conversation
This will allow to beat other event handlers that calls `event.stopPropagation()`.
|
I need some help testing this. I already checked that the existing styleguide functionality works in Chrome, Firefox and Safari. |
luis-almeida
left a comment
luis-almeida
left a comment
There was a problem hiding this comment.
Looks solid! 👍
Added a question but related to the unit test.
| it('removes click outside event listener', () => { | ||
| const removeEventListenerSpy = jest.fn(); | ||
|
|
||
| document.removeEventListener = removeEventListenerSpy; |
There was a problem hiding this comment.
Should we restore the original method not to temper with future tests?
Not sure what's the cleanest way do this with jest but with sinon it looks something like this:
const removeEventListenerSpy = sinon.spy(document, 'removeEventListener');
// ... assert
removeEventListenerSpy.restore();There was a problem hiding this comment.
It is a good point, I just copied it from the old test. I'll just fix that.
There was a problem hiding this comment.
The update looks good 👍
| attachClickOutsideHandler() { | ||
| this.getDocuments().forEach(doc => { | ||
| doc.addEventListener('mousedown', this.handleOutsideMouseDown); | ||
| doc.addEventListener('mousedown', this.handleOutsideMouseDown, true); |
There was a problem hiding this comment.
It's always friendly to provide a quick inline comment reminder for boolean parameters: i.e. doc.addEventListener(..., true /* useCapture*/);
|
Are you able to link this locally within Guide to ensure that it works? We can also publish a |
Seriously I don't get coveralls to me you've added tests yet almost every pr it slightly declines |
I tested is quite heavily in the styleguide with different kinds of iframes. But I haven't tested the normal functionality with IE and Edge. I'll try to see how hard it is to link into the guide-chrome, but my guess is that it is not easy. |
|
I tested the styleguide in IE 11 and Edge as well. I believe this should be ready to merge. |
|
Perfect, I'll take it through the release cycle.
With yarn it's gotten a lot easier. Just |
|
I know how to link stuff. But it needs to get through 3 steps of bundling, so that was what I thought might be challenging 😜 |
|
Thanks 💯 |
|
Just published to the registry Cheers 🥂 |
Fixes: #137
Description
Take three, we had a pretty good solution in our previous menu component to handle closing the menu when clicked outside of the containers. This PR mirrors the functionality even closer.
Detail
event.stopPropagation()Checklist