Skip to content
This repository has been archived by the owner on Nov 8, 2018. It is now read-only.

popover should close on clicking somewhere else #1521

Merged
merged 1 commit into from
Jun 6, 2016

Conversation

tahaalibra
Copy link
Contributor

Fix: the popover should close when clicking somewhere else.

@Gomez @ChristophWurst

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @ChristophWurst and @Gomez to be potential reviewers

},
toggleMenu: function(e) {
e.preventDefault();
this.menuShown = !this.menuShown;
this.listenTo(Radio.ui, 'document:click', function(event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, doesn't that mean everytime the menu is toggled, another event handler is registered?

Copy link
Contributor

Choose a reason for hiding this comment

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

add a onShow method and move that event handler registration in there. That method is called by Marionette once a view has finished rendering and is added to the DOM.

@tahaalibra
Copy link
Contributor Author

tahaalibra commented Jun 6, 2016

@ChristophWurst putting the listening in initialize function...does this solve the problem

@ChristophWurst
Copy link
Contributor

@ChristophWurst putting the listening in initialize function...does this solve the problem

Well, did you try the onShow approach? I'm in favor of that since the initialize method might be called even if the view is not attached to the DOM, which is error-prone…

@tahaalibra
Copy link
Contributor Author

tahaalibra commented Jun 6, 2016

@ChristophWurst using onShow meths , thanks for help :) !!...

@@ -3,6 +3,10 @@ All notable changes to this project will be documented in this file.

## 0.5.2 - unreleased

### Fixed
- Close popover on clicking somewhere else
Copy link
Contributor

Choose a reason for hiding this comment

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

awesome :-)

@ChristophWurst
Copy link
Contributor

👍 very nice!

@ChristophWurst
Copy link
Contributor

oh, now there are 4 commits. Can you squash them with git rebase -i HEAD~4? Thanks

@ChristophWurst
Copy link
Contributor

ah, you're fast 🚀

close popover fix

close popover fix

close popover fix

close popover fix
@ChristophWurst ChristophWurst merged commit 4d23ece into master Jun 6, 2016
@ChristophWurst ChristophWurst deleted the accounts-popover-fix branch June 6, 2016 12:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants