Panel: panelclose event does not work #7260

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@cgack
Contributor

cgack commented Mar 19, 2014

panelclose is not bound to document after _openPanel

Fixes gh-7058

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Apr 24, 2014

Member

This cant be removed in this way or the panel will open every time a panel closes. The problem here is we are removing all handlers instead of just the one we are attaching. Also we should see if there is a reason not to use one here instead of on or why we are using on and off and not _on and _off which makes sure we only get our own handlers and that they are removed properly on destroy.

Member

arschmitz commented Apr 24, 2014

This cant be removed in this way or the panel will open every time a panel closes. The problem here is we are removing all handlers instead of just the one we are attaching. Also we should see if there is a reason not to use one here instead of on or why we are using on and off and not _on and _off which makes sure we only get our own handlers and that they are removed properly on destroy.

@cgack

This comment has been minimized.

Show comment
Hide comment
@cgack

cgack Apr 24, 2014

Contributor

@arschmitz switched to using _off and _on as we discussed

Contributor

cgack commented Apr 24, 2014

@arschmitz switched to using _off and _on as we discussed

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Apr 24, 2014

Member

@cgack please see http://api.jqueryui.com/jQuery.widget/#method-_on for proper usage of _on right now this is not working. This makes me worry also about that we are not testing this properly because all tests pass yet if you go to the responsive panels page in the demos on your fork and open the left panel then run $("#add-form").panel("open"); in the console you will se this results in an error but on master it works as intended. We should add a test to prevent regressions.

Member

arschmitz commented Apr 24, 2014

@cgack please see http://api.jqueryui.com/jQuery.widget/#method-_on for proper usage of _on right now this is not working. This makes me worry also about that we are not testing this properly because all tests pass yet if you go to the responsive panels page in the demos on your fork and open the left panel then run $("#add-form").panel("open"); in the console you will se this results in an error but on master it works as intended. We should add a test to prevent regressions.

js/widgets/panel.js
+ self._on( self.document, {
+ "panelclose": function() {
+ _openPanel();
+ }

This comment has been minimized.

@arschmitz

arschmitz May 29, 2014

Member

this should be "panelclose": "_openPanel"

@arschmitz

arschmitz May 29, 2014

Member

this should be "panelclose": "_openPanel"

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz May 29, 2014

Member

Its strange this passed all the tests i can't see how this would work in its current form.

Member

arschmitz commented May 29, 2014

Its strange this passed all the tests i can't see how this would work in its current form.

@cgack

This comment has been minimized.

Show comment
Hide comment
@cgack

cgack May 29, 2014

Contributor

It must have been magic. I'll update the branch with the change

Contributor

cgack commented May 29, 2014

It must have been magic. I'll update the branch with the change

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 29, 2014

Coverage Status

Coverage increased (+0.02%) when pulling d836518 on cgack:7058-panelclose-event into fec1e85 on jquery:master.

Coverage Status

Coverage increased (+0.02%) when pulling d836518 on cgack:7058-panelclose-event into fec1e85 on jquery:master.

@cgack

This comment has been minimized.

Show comment
Hide comment
@cgack

cgack May 29, 2014

Contributor

@arschmitz I updated this PR with a new commit for you to review whenever

Contributor

cgack commented May 29, 2014

@arschmitz I updated this PR with a new commit for you to review whenever

tests/unit/panel/index.html
@@ -118,5 +128,6 @@ <h1 id="demo-links">Panels</h1>
<a href="#" data-nstest-rel="back">Back</a>
</div>
+

This comment has been minimized.

@arschmitz

arschmitz Jun 13, 2014

Member

remove extra line here

@arschmitz

arschmitz Jun 13, 2014

Member

remove extra line here

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jun 13, 2014

Member

@cgack other then that one line this is good to go. Can you fix that and rebase please. When your done ping me on irc and I will land this.

Member

arschmitz commented Jun 13, 2014

@cgack other then that one line this is good to go. Can you fix that and rebase please. When your done ping me on irc and I will land this.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 13, 2014

Coverage Status

Coverage increased (+0.68%) when pulling f93385d on cgack:7058-panelclose-event into fec1e85 on jquery:master.

Coverage Status

Coverage increased (+0.68%) when pulling f93385d on cgack:7058-panelclose-event into fec1e85 on jquery:master.

@cgack

This comment has been minimized.

Show comment
Hide comment
@cgack

cgack Jun 13, 2014

Contributor

@arschmitz fixed the one line and rebased into a single commit. fire at will.

Contributor

cgack commented Jun 13, 2014

@arschmitz fixed the one line and rebased into a single commit. fire at will.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 13, 2014

Coverage Status

Coverage decreased (-6.49%) when pulling 4c2ed33 on cgack:7058-panelclose-event into 66efe74 on jquery:master.

Coverage Status

Coverage decreased (-6.49%) when pulling 4c2ed33 on cgack:7058-panelclose-event into 66efe74 on jquery:master.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jun 14, 2014

Member

@cgack sorry this is my fault but when we say squash we really normally mean fixup otherwise you get a massive and not very useful commit message cgack/jquery-mobile@4c2ed33 can you just clean this up. Also you forgot Closes gh-7260 to close this pr Thanks!

Member

arschmitz commented Jun 14, 2014

@cgack sorry this is my fault but when we say squash we really normally mean fixup otherwise you get a massive and not very useful commit message cgack/jquery-mobile@4c2ed33 can you just clean this up. Also you forgot Closes gh-7260 to close this pr Thanks!

cgack added a commit to cgack/jquery-mobile that referenced this pull request Jun 14, 2014

@cgack

This comment has been minimized.

Show comment
Hide comment
@cgack

cgack Jun 14, 2014

Contributor

ha! @arschmitz I thought it was a little crazy to have all that noise in there - Updated with a new cleaner message - hopefully this works

Contributor

cgack commented Jun 14, 2014

ha! @arschmitz I thought it was a little crazy to have all that noise in there - Updated with a new cleaner message - hopefully this works

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jun 14, 2014

Member

@cgack can you make the commit message what you did instead of the problem you fixed.

Member

arschmitz commented Jun 14, 2014

@cgack can you make the commit message what you did instead of the problem you fixed.

@cgack

This comment has been minimized.

Show comment
Hide comment
@cgack

cgack Jun 14, 2014

Contributor

@arschmitz sure! how's it look now?

Contributor

cgack commented Jun 14, 2014

@arschmitz sure! how's it look now?

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jun 14, 2014

Member

👍

Member

arschmitz commented Jun 14, 2014

👍

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 14, 2014

Coverage Status

Coverage decreased (-6.55%) when pulling 3728bc8 on cgack:7058-panelclose-event into 66efe74 on jquery:master.

Coverage Status

Coverage decreased (-6.55%) when pulling 3728bc8 on cgack:7058-panelclose-event into 66efe74 on jquery:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 14, 2014

Coverage Status

Coverage decreased (-6.68%) when pulling 1f763bc on cgack:7058-panelclose-event into 66efe74 on jquery:master.

Coverage Status

Coverage decreased (-6.68%) when pulling 1f763bc on cgack:7058-panelclose-event into 66efe74 on jquery:master.

@arschmitz arschmitz closed this in 3df9b3b Jun 14, 2014

arschmitz added a commit that referenced this pull request Jun 16, 2014

agcolom added a commit to agcolom/jquery-mobile that referenced this pull request Nov 26, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment