Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Toolbar: Back button correctly appears/disappears on external toolbar #7188

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

gabrielschulhof commented Feb 27, 2014

This commit makes the following modifications:

  1. Rename _addBackButton() to _updateBackButton() and move the decision making
    as to whether to add or remove a back button from _setOptions() into
    _updateBackButton().
  2. Call _updateBackButton() from refresh() as well. This will cause the back
    button to be updated whenever the page changes, because the "pageshow"
    handler is hooked up to refresh().
  3. Modify _addHeaderButtonClasses to not recognize the back button as a left
    button.
  4. Modify the preconditions for adding a button to include an alternative to
    checking the page's URL for instances where there is no page present - i.e.
    when the toolbar is external. In such cases one must add a back button if
    the active item on the history stack is not the first item.

Fixes gh-7091

Coverage Status

Coverage increased (+0.25%) when pulling bc3660f on 7091-back-button-on-external-toolbar into 53a11b1 on master.

@gabrielschulhof gabrielschulhof added this to the 1.4.3 milestone Feb 27, 2014

Owner

arschmitz commented Apr 23, 2014

should change this to fixes gh-6905 which #7091 is a dup of

@arschmitz arschmitz commented on an outdated diff Apr 23, 2014

js/widgets/toolbar.js
theme = options.backBtnTheme || options.theme;
- $( "<a role='button' href='javascript:void(0);' " +
- "class='ui-btn ui-corner-all ui-shadow ui-btn-left " +
- ( theme ? "ui-btn-" + theme + " " : "" ) +
- "ui-toolbar-back-btn ui-icon-carat-l ui-btn-icon-left' " +
- "data-" + $.mobile.ns + "rel='back'>" + options.backBtnText + "</a>" )
- .prependTo( this.element );
+ // We add a back button under the following circumstances:
+ // - the option to do so is on
@arschmitz

arschmitz Apr 23, 2014

Owner

All comments should start with a capital letter the same goes for below comments.

gabrielschulhof added some commits Feb 27, 2014

Toolbar: Back button correctly appears/disappears on external toolbar
This commit makes the following modifications:
1. Rename _addBackButton() to _updateBackButton() and move the decision making
   as to whether to add or remove a back button from _setOptions() into
   _updateBackButton().

2. Call _updateBackButton() from refresh() as well. This will cause the back
   button to be updated whenever the page changes, because the "pageshow"
   handler is hooked up to refresh().

3. Modify _addHeaderButtonClasses to not recognize the back button as a left
   button.

4. Modify the preconditions for adding a button to include an alternative to
   checking the page's URL for instances where there is no page present - i.e.
   when the toolbar is external. In such cases one must add a back button if
   the active item on the history stack is not the first item.

Closes gh-7188
Fixes gh-6950
Contributor

gabrielschulhof commented Apr 23, 2014

@arschmitz I've fixed the comments and the commit message.

@arschmitz arschmitz commented on an outdated diff Apr 23, 2014

js/widgets/toolbar.js
@@ -103,24 +97,64 @@ define( [
// Deprecated in 1.4. As from 1.5 ui-btn-left/right classes have to be present in the markup.
_addHeaderButtonClasses: function() {
var $headeranchors = this.element.children( "a, button" );
- this.leftbtn = $headeranchors.hasClass( "ui-btn-left" );
+
+ // Do not mistake a back button for a left toolbar button
+ this.leftbtn = $headeranchors.hasClass( "ui-btn-left" ) &&
+ !$headeranchors.hasClass( "ui-toolbar-back-btn" );
@arschmitz

arschmitz Apr 23, 2014

Owner

Please fix hungarian notation.

@arschmitz arschmitz commented on an outdated diff Apr 23, 2014

js/widgets/toolbar.js
var options = this.options,
+ backButton = this.element.find( ".ui-toolbar-back-btn" ),
@arschmitz

arschmitz Apr 23, 2014

Owner

we should be saving a reference to the button when it is created not querying for it each time.

@arschmitz arschmitz commented on an outdated diff Apr 23, 2014

js/widgets/toolbar.js
+
+ // If the toolbar is internal the page's URL must differ from the hash
+ ( this.page[ 0 ].getAttribute( "data-" + $.mobile.ns + "url" ) !==
+ $.mobile.path.stripHash( location.hash ) ) :
+
+ // Otherwise, if the toolbar is external there must be at least one
+ // history item to which one can go back
+ ( $.mobile.navigate && $.mobile.navigate.history &&
+ $.mobile.navigate.history.activeIndex > 0 ) ) &&
+
+ // The toolbar does not have a left button
+ !this.leftbtn ) {
+
+ // Skip back button creation if one is already present
+ if ( backButton.length === 0 ) {
+ $( "<a role='button' href='javascript:void(0);' " +
@arschmitz

arschmitz Apr 23, 2014

Owner

save a reference to the button on the widget here for reuse

@arschmitz arschmitz commented on an outdated diff Apr 23, 2014

js/widgets/toolbar.js
theme = options.backBtnTheme || options.theme;
- $( "<a role='button' href='javascript:void(0);' " +
- "class='ui-btn ui-corner-all ui-shadow ui-btn-left " +
- ( theme ? "ui-btn-" + theme + " " : "" ) +
- "ui-toolbar-back-btn ui-icon-carat-l ui-btn-icon-left' " +
- "data-" + $.mobile.ns + "rel='back'>" + options.backBtnText + "</a>" )
- .prependTo( this.element );
+ // We add a back button only if the option to do so is on
+ if ( this.options.addBackBtn &&
+
+ // This must also be a header toolbar
+ this.role === "header" &&
+
+ // There must be multiple pages in the DOMN
@arschmitz

arschmitz Apr 23, 2014

Owner

DOM not DOMN

@arschmitz arschmitz commented on an outdated diff Apr 23, 2014

js/widgets/toolbar.js
+ // The toolbar does not have a left button
+ !this.leftbtn ) {
+
+ // Skip back button creation if one is already present
+ if ( backButton.length === 0 ) {
+ $( "<a role='button' href='javascript:void(0);' " +
+ "class='ui-btn ui-corner-all ui-shadow ui-btn-left " +
+ ( theme ? "ui-btn-" + theme + " " : "" ) +
+ "ui-toolbar-back-btn ui-icon-carat-l ui-btn-icon-left' " +
+ "data-" + $.mobile.ns + "rel='back'>" + options.backBtnText + "</a>" )
+ .prependTo( this.element );
+ }
+
+ // If we are not adding a back button, then remove the one present, if any
+ } else {
+ backButton.remove();
@arschmitz

arschmitz Apr 23, 2014

Owner

rather then remove we should either hide or detach it will be common for someone to repeatedly go from the first page to another page and back again we should not recreate the element each time.

gabrielschulhof added some commits Apr 23, 2014

Owner

arschmitz commented Jun 5, 2014

👍

gabrielschulhof added a commit that referenced this pull request Jun 6, 2014

Toolbar: Back button correctly appears/disappears on external toolbar
This commit makes the following modifications:
1. Rename _addBackButton() to _updateBackButton() and move the decision making
   as to whether to add or remove a back button from _setOptions() into
   _updateBackButton().

2. Call _updateBackButton() from refresh() as well. This will cause the back
   button to be updated whenever the page changes, because the "pageshow"
   handler is hooked up to refresh().

3. Modify _addHeaderButtonClasses to not recognize the back button as a left
   button.

4. Modify the preconditions for adding a button to include an alternative to
   checking the page's URL for instances where there is no page present - i.e.
   when the toolbar is external. In such cases one must add a back button if
   the active item on the history stack is not the first item.

(cherry picked from commit b0685b3)

Closes gh-7188
Fixes gh-6950

@gabrielschulhof gabrielschulhof deleted the 7091-back-button-on-external-toolbar branch Jun 6, 2014

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

Toolbar: Back button correctly appears/disappears on external toolbar
This commit makes the following modifications:
1. Rename _addBackButton() to _updateBackButton() and move the decision making
   as to whether to add or remove a back button from _setOptions() into
   _updateBackButton().

2. Call _updateBackButton() from refresh() as well. This will cause the back
   button to be updated whenever the page changes, because the "pageshow"
   handler is hooked up to refresh().

3. Modify _addHeaderButtonClasses to not recognize the back button as a left
   button.

4. Modify the preconditions for adding a button to include an alternative to
   checking the page's URL for instances where there is no page present - i.e.
   when the toolbar is external. In such cases one must add a back button if
   the active item on the history stack is not the first item.

Closes gh-7188
Fixes gh-6950
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment