Fixed Toolbar: removing popup removes ui-page-header-fixed class #7252

cgack · 2014-03-17T14:49:16Z

arschmitz · 2014-05-29T13:55:31Z

js/widgets/fixedToolbar.js

@@ -278,11 +278,18 @@ define( [ "jquery", "../widget", "../core", "../animationComplete", "../navigati

 		_destroy: function() {
 			var $el = this.element,
-				header = $el.hasClass( "ui-header" );
+				header = $el.hasClass( "ui-header" ),
+				hasFixedChildren = $el.closest( ".ui-page" ).children( ".ui-header-fixed" ).length > 0;


line length

arschmitz · 2014-05-29T14:00:23Z

Throughout both js files lots of hungarian notation used the style guide does not allow for hungarian notation.

cgack · 2014-05-29T19:52:58Z

@arschmitz I updated the pr with another commit. feel free to review whenever.

coveralls · 2014-05-29T22:17:52Z

Coverage increased (+0.33%) when pulling 41ad1ab on cgack:6987-destroyed-popup-removes-page-header-fixed into 923c1f8 on jquery:master.

cgack · 2014-06-02T22:30:46Z

@arschmitz I have updated this pr. I had left the $el alone originally because it had previously existed, but I see the value in removing those when we are in there to clean it up. There are others in that file - should I make an issue to clean up the hungarian notation and get that fixed as well? Also, I switched away from using $.mobile.pageContainer as well.

arschmitz · 2014-06-02T22:32:55Z

Yes I only mentioned it here because its one your actually using. I think a new issue to clean this file up is the right way to go for the rest.

coveralls · 2014-06-02T22:56:11Z

Coverage increased (+0.56%) when pulling 50e7c8b on cgack:6987-destroyed-popup-removes-page-header-fixed into 923c1f8 on jquery:master.

arschmitz · 2014-06-06T02:05:33Z

@cgack it looks like you don't have your full name ( only have cgack ) in your git config can you update this please.

arschmitz · 2014-06-06T02:18:18Z

Iv just realized there is a much bigger issue here, that any time you destroy any toolbar its treating it as a fixed toolbar. Popup's toolbar is not fixed and so the logic from the fixedtoolbar extension should not even be running in the first place. Also we are never calling _super() here to allow toolbar destroy to run. Everything in destroy should be in an if statement to make sure position:fixed and we should always be calling _super() and make sure there is no unnecessarily duplicated logic between toolbar destroy and fixedToolbar.

cgack · 2014-06-06T13:24:37Z

@arschmitz I've updated my git config on that machine so it'll be ready for the next push.
it seems that toolbar by itself has no destroy method, unless I'm missing it. Should this be a new issue to add that functionality and then allow for _super() here

arschmitz · 2014-06-06T13:29:08Z

@cgack ahh good point ok so destroy here is a mess lol.

This is all really the same issue though that toolbars _destroy ( what should be actually getting called here ) is missing.

So I think we need to just update the issue and fix the real problem here.

cgack · 2014-06-06T13:33:44Z

Okay - i'll work on getting _destroy added to toolbar and add it to this PR then

arschmitz · 2014-06-06T13:34:57Z

@cgack Thanks sorry to initially lead you down the wrong path!

cgack · 2014-06-06T14:18:21Z

@arschmitz Thinking maybe we should push this to milestone 1.4.4 so that adding _destroy doesn't hold up 1.4.3

arschmitz · 2014-06-06T15:11:28Z

@cgack you are probably right unless you get to it in next few days

cgack · 2014-06-06T20:32:06Z

@arschmitz I went ahead and gave it a go at adding _destroy to toolbar.js. There seems to be a bunch of stuff going on in there and I think I hit it all but a review is most definitely in order here. I wasn't sure what we did about removing aria roles or things like that in _destroy methods, also not sure how to handle if the footer has been appended to body if we could ever know where to put it back.

coveralls · 2014-06-06T20:56:07Z

Coverage increased (+0.81%) when pulling 95579f0 on cgack:6987-destroyed-popup-removes-page-header-fixed into 923c1f8 on jquery:master.

gabrielschulhof · 2014-06-09T12:03:21Z

@cgack without having read any of the thread except the part about where to put it back, I can tell you that, in popup, where I also move this.element from one place to another, I put a hidden div into the location from which I remove this.element, so that during _destroy() I'll have a place to put it back - i.e., placeholder.before( this.element ).remove().

jaspermdegroot · 2014-06-11T11:52:12Z

This PR is meant to fix issues that have a 1.4.x milestone. That brings up the discussion again if adding a missing method can be considered as a bug fix and land in a maintenance release or a new feature which should wait until next version. IINM the outcome of previous discussions was that adding methods always has to wait until next version.

I suggest we open a new ticket for Toolbar widget review (already on our roadmap for 1.5) and re-milestone #6987 and #6939 to 1.5.

arschmitz · 2014-06-11T11:55:12Z

@uGoMobi its always been there just not correctly implemented

jaspermdegroot · 2014-06-11T12:06:43Z

Ok, found the _destroy() in the fixedToolbar extension. So we won't be adding a new method. Then we can fix it for 1.4.3 or 1.4.4.

arschmitz · 2014-08-22T02:51:26Z

js/widgets/toolbar.js

+				.removeAttr( "aria-level" );
+
+			if ( this.role === "header" ) {
+				this.element.children( "a, button" ).removeClass( "ui-btn-left ui-btn-right" );


I think at least for 1.4.4 we need to remove all the button classes since the button is enhanced as part of the enhancement process on the toolbar it should be destroyed as well.

cgack · 2014-08-28T17:06:16Z

@arschmitz went the the PR and made the requested changes. Thanks for the review!

arschmitz · 2014-08-29T02:47:15Z

js/widgets/fixedToolbar.js

+				hasFixedChildren = $.mobile.document.find( ".ui-header-fixed" )
+										.length > 0;
+				toolbarClasses =  "ui-header-fixed ui-footer-fixed ui-header-fullscreen in out";
+				toolbarClasses += " ui-footer-fullscreen fade slidedown slideup ui-fixed-hidden";


Rather then doing += do

toolbarClasses = "ui-header-fixed ui-footer-fixed ui-header-fullscreen in out" + " ui-footer-fullscreen fade slidedown slideup ui-fixed-hidden";

cgack · 2014-08-29T18:30:36Z

@arschmitz I made some more adjustments per your PR comments. Thanks for reviewing!

arschmitz · 2014-09-04T12:17:47Z

js/widgets/fixedToolbar.js

+			if ( this.options.position === "fixed" ) {
+				hasFixed = $(  "body>.ui-" + this.role + "-fixed" )
+							.add( page )
+							.find( ".ui-" + this.options.role + "-fixed" )


ok while this works and i suggested it i think i typed it with a parentheses in the wrong place :/
should be

$( "body>.ui-" + this.role + "-fixed" ).add( page.find( ".ui-" + this.options.role + "-fixed" ) ).not( this.element ).length > 0;

same for fullscreen checkbelow

cgack · 2014-09-04T13:16:07Z

@arschmitz I have modified the PR. Thanks for the review!

arschmitz · 2014-09-04T13:52:00Z

@cgack looks good 👍 will land shortly

Fixes jquery-archivegh-6987 Fixes jquery-archivegh-6939

cgack · 2014-09-04T19:01:57Z

@arschmitz rebased with master and updated - hopefully the line endings are good now

Fixes gh-6987 Fixes gh-6939 Closes gh-7252 (cherry picked from commit 3efe6fe)

Fixes jquery-archivegh-6987 Fixes jquery-archivegh-6939 Closes jquery-archivegh-7252

gabrielschulhof added the Component: Toolbar label Mar 20, 2014

gabrielschulhof added this to the 1.4.3 milestone Mar 20, 2014

gabrielschulhof assigned arschmitz Mar 20, 2014

gabrielschulhof added the Action: PR needs reviewing label Apr 24, 2014

arschmitz reviewed May 29, 2014
View reviewed changes

arschmitz reviewed Aug 22, 2014
View reviewed changes

arschmitz reviewed Aug 29, 2014
View reviewed changes

arschmitz reviewed Sep 4, 2014
View reviewed changes

arschmitz added Action: PR needs modification and removed Action: PR needs reviewing labels Sep 4, 2014

cgack force-pushed the 6987-destroyed-popup-removes-page-header-fixed branch from 0c46cc9 to 2aaac08 Compare September 4, 2014 18:45

Toolbar: removing popup removes page-header-fixed class

1cc1611

Fixes jquery-archivegh-6987 Fixes jquery-archivegh-6939

cgack force-pushed the 6987-destroyed-popup-removes-page-header-fixed branch from 2aaac08 to 1cc1611 Compare September 4, 2014 19:01

arschmitz closed this in 3efe6fe Sep 4, 2014

arschmitz pushed a commit that referenced this pull request Sep 12, 2014

Toolbar: Sorts our proper destroy methods for toolbar and fixedtoolbar

98ece81

Fixes gh-6987 Fixes gh-6939 Closes gh-7252 (cherry picked from commit 3efe6fe)

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

Toolbar: Sorts our proper destroy methods for toolbar and fixedtoolbar

5726407

Fixes jquery-archivegh-6987 Fixes jquery-archivegh-6939 Closes jquery-archivegh-7252

jaspermdegroot removed the Action: PR needs modification label Aug 20, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Toolbar: removing popup removes ui-page-header-fixed class #7252

Fixed Toolbar: removing popup removes ui-page-header-fixed class #7252

cgack commented Mar 17, 2014

arschmitz May 29, 2014

arschmitz commented May 29, 2014

cgack commented May 29, 2014

coveralls commented May 29, 2014

cgack commented Jun 2, 2014

arschmitz commented Jun 2, 2014

coveralls commented Jun 2, 2014

arschmitz commented Jun 6, 2014

arschmitz commented Jun 6, 2014

cgack commented Jun 6, 2014

arschmitz commented Jun 6, 2014

cgack commented Jun 6, 2014

arschmitz commented Jun 6, 2014

cgack commented Jun 6, 2014

arschmitz commented Jun 6, 2014

cgack commented Jun 6, 2014

coveralls commented Jun 6, 2014

gabrielschulhof commented Jun 9, 2014

jaspermdegroot commented Jun 11, 2014

arschmitz commented Jun 11, 2014

jaspermdegroot commented Jun 11, 2014

arschmitz Aug 22, 2014

cgack commented Aug 28, 2014

arschmitz Aug 29, 2014

cgack commented Aug 29, 2014

arschmitz Sep 4, 2014

cgack commented Sep 4, 2014

arschmitz commented Sep 4, 2014

cgack commented Sep 4, 2014

Fixed Toolbar: removing popup removes ui-page-header-fixed class #7252

Fixed Toolbar: removing popup removes ui-page-header-fixed class #7252

Conversation

cgack commented Mar 17, 2014

arschmitz May 29, 2014

Choose a reason for hiding this comment

arschmitz commented May 29, 2014

cgack commented May 29, 2014

coveralls commented May 29, 2014

cgack commented Jun 2, 2014

arschmitz commented Jun 2, 2014

coveralls commented Jun 2, 2014

arschmitz commented Jun 6, 2014

arschmitz commented Jun 6, 2014

cgack commented Jun 6, 2014

arschmitz commented Jun 6, 2014

cgack commented Jun 6, 2014

arschmitz commented Jun 6, 2014

cgack commented Jun 6, 2014

arschmitz commented Jun 6, 2014

cgack commented Jun 6, 2014

coveralls commented Jun 6, 2014

gabrielschulhof commented Jun 9, 2014

jaspermdegroot commented Jun 11, 2014

arschmitz commented Jun 11, 2014

jaspermdegroot commented Jun 11, 2014

arschmitz Aug 22, 2014

Choose a reason for hiding this comment

cgack commented Aug 28, 2014

arschmitz Aug 29, 2014

Choose a reason for hiding this comment

cgack commented Aug 29, 2014

arschmitz Sep 4, 2014

Choose a reason for hiding this comment

cgack commented Sep 4, 2014

arschmitz commented Sep 4, 2014

cgack commented Sep 4, 2014