Close New Rule dropdown on inline editor close or editor resize #6056
Conversation
ghost
assigned 
|
@njx I'm back and waiting for review comments on this pull request. |
| dropdownEventHandler = new DropdownEventHandler($dropdown, _onSelect, _cleanupDropdown); | ||
| dropdownEventHandler.open(); | ||
|
|
||
| $dropdown.focus(); | ||
|
|
||
| $(hostEditor).on("scroll", _onScroll); | ||
| $("html").on("click", _closeDropdown); |
There was a problem hiding this comment.
I don't know if we do this for other dropdowns, but I wonder if it would be better to add a capture handler to html for this case. That would make it so we don't have to special-case things like clicking on the close button. With this implementation, we don't end up closing the dropdown until after the inline editor animates closed, which looks a little funny. It also means that there might be other cases where we don't close the dropdown (i.e., if you click on some other element that stops propagation in its click handler).
I guess the other question to ask is why the click handler in the close button on the inline editor stops propagation. @RaymondLim - I think you reviewed that code originally - do you know why?
There was a problem hiding this comment.
Fixed. This now closes dropdown before (at same time as) inline editor closes.
There was a problem hiding this comment.
I reverted that commit.
Note: that code did not change as a part of this pull request -- I only moved that line to group the event handlers together.
There was a problem hiding this comment.
I think we could still have the capture handler - it just has to check if the event target is in the dropdown, and if so, don't handle it. But yeah, we can handle that as a separate low-pri bug.
|
Reviewed. I would be fine merging this as-is, but just wanted to raise the questions in the comment above. Perhaps the more general question (about how we deal with clicks closing the dropdown) should be handled as part of the popup management cleanup we haven't gotten around to yet: https://trello.com/c/o7jf6s6W |
|
Ready for re-review. |
|
Oops...that doesn't seem to work because now clicks within the dropdown itself aren't handled. |
This reverts commit 65cc4dd.
|
Doh -- I guess i didn't test that. I reverted that commit. |
Close New Rule dropdown on inline editor close or editor resize
|
Merged. I'll file a separate issue on using a capture handler. |
This is for #6005
With this change, the New Rule button dropdown list is now dismissed for these additional cases: