Fix tabs deeplink history to work with back button within page #9674

kball · 2017-01-18T23:15:10Z

I noticed while working on a lesson on tabs deep linking that the history feature was a little flaky - while it added the history to the URL, clicking back did not appropriately open the previous tab. This fixes this bug by moving the deep link logic into a function and calling it on popstate.

kball · 2017-01-18T23:16:10Z

@ahebrank and @xhezairi you have both worked in this code recently, can you review this PR? Thanks

ncoden · 2017-01-18T23:52:26Z

yarn.lock

-dom-serializer@0, dom-serializer@~0.1.0:
-  version "0.1.0"
-  resolved "https://registry.yarnpkg.com/dom-serializer/-/dom-serializer-0.1.0.tgz#073c697546ce0780ce23be4a28e293e40bc30c82"
+dom-serializer@0, dom-serializer@~0.0.0:


We should update yarn time to time, but not in this PR.

Good point, incidental, will fix.

ncoden · 2017-01-18T23:52:57Z

js/foundation.tabs.js

@@ -365,6 +371,7 @@ class Tabs {
      }
    }

+    $(window).off('popstate', this._checkDeepLink);


No check for this.options.deepLink ?

we can, but shouldn't matter. If the handler isn't there it's a noop

I think we should explicitly show that a piece of code is only required by an option.

One year later: that was a stupid idea.

Options can changes, listeners can be created in complex tests, remplaced, created dynamically. Here we only want to clear everything. No need for that check.

True. So do we need further changes here?

No, don't worry. I seen that while working on #11077

ncoden · 2017-01-18T23:54:02Z

js/foundation.tabs.js

@@ -114,9 +93,32 @@ class Tabs {
      }
    }

+    this._checkDeepLink = this._checkDeepLink.bind(this);


So that we can

Count on this within the function being a reference to the plugin

have a single representation of the function (rather than doing bind at the time we add the handler), letting us remove it using off

ahebrank · 2017-01-19T02:25:41Z

Thanks, haven't been eating my dogfood and didn't see this glitch.

Two minor armchair things:

I find the bound function's name reuse confusing, so I'd probably try anonymizing it.
move the options check to the top of the new function so you can call it with impunity and get rid of a couple of ifs elsewhere.

So, at L96:

this._checkDeepLink = () => {
    if (!this.options.deepLink) { 
      return; 
    }
    var anchor = window.location.hash;
    //need a hash and a relevant anchor in this tabset
    if(anchor.length) {
      var $link = this.$element.find('[href="'+anchor+'"]');
      if ($link.length) {
        this.selectTab($(anchor));

        //roll up a little to show the titles
        if (this.options.deepLinkSmudge) {
          var offset = this.$element.offset();
          $('html, body').animate({ scrollTop: offset.top }, this.options.deepLinkSmudgeDelay);
        }

        /**
          * Fires when the zplugin has deeplinked at pageload
          * @event Tabs#deeplink
          */
         this.$element.trigger('deeplink.zf.tabs', [$link, $(anchor)]);
       }
     }
};

The only other thing I'm wondering is if the popstate event should check options.updateHistory. I'm thinking the answer is no, because if this is false the back button shouldn't be doing anything relevant to the tabs anyway. But just in case, L137 would become:

    if(this.options.updateHistory) {
      $(window).on('popstate', this._checkDeepLink);
    }

kball · 2017-01-19T17:01:07Z

@ahebrank can you clarify a little what you meant by anonymizing it? The reason I set it up this way was to be able to remove explicitly this handler on destroy, so as not to have to mess with other handlers...

That said, maybe we should implement a global popstate handler similar to how we handle resize/scroll. Thoughts @coreysyms?

ahebrank · 2017-01-19T17:25:58Z

@kball, here's a diff that should clarify: fix-tabs-history...ahebrank:fix-tabs-history

kball · 2017-01-19T17:58:58Z

Righto, makes sense. Will push up an update later today. Thanks @ahebrank

designerno1 · 2017-01-19T21:31:24Z

the updateHistory got a little problem in _handleTabChange if it get's handled by the deeplink check it shouldn't pushState again.
otherwise we got the panel hash two times in history after changing the tab on popstate or init
maybe adding a parameter to the _handleTabChange could do the job if it's set by the checkdeeplink do not pushstate

kball · 2017-01-20T22:08:42Z

@designerno1 @ahebrank @ncoden Updated to address all of the concerns raised, please take another look thanks

designerno1 · 2017-01-20T23:14:44Z

looks good to me

ahebrank · 2017-01-21T15:09:18Z

js/foundation.tabs.js

@@ -297,7 +304,7 @@ class Tabs {
   * @param {jQuery | String} elem - jQuery object or string of the id of the pane to display.
   * @function


need a @param definition, like:

@param {boolean} - browser has already handled a history update

ahebrank · 2017-01-21T15:09:56Z

js/foundation.tabs.js

@@ -206,7 +213,7 @@ class Tabs {
   * @fires Tabs#change


@param {boolean} - browser has already handled a history update

ahebrank · 2017-01-21T15:11:29Z

Needs a comment for the new function parameter, but otherwise thumbs up.

Fix tabs deeplink history to work with back button within page

Fix tabs deeplink history to work with back button within page

b85975d

ncoden reviewed Jan 18, 2017

View reviewed changes

Restore yarn.lock to where it is on develop

5c6face

Update deeplink approach to address concerns in #9674

1e50cea

ahebrank reviewed Jan 21, 2017

View reviewed changes

Add doc comments for new historyHandled param

333ac06

kball merged commit fd6cdc7 into develop Jan 23, 2017

kball added a commit that referenced this pull request Jan 23, 2017

Merge pull request #9674 from zurb/fix-tabs-history

9659c15

Fix tabs deeplink history to work with back button within page

kball deleted the fix-tabs-history branch April 20, 2017 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tabs deeplink history to work with back button within page #9674

Fix tabs deeplink history to work with back button within page #9674

kball commented Jan 18, 2017

kball commented Jan 18, 2017

ncoden Jan 18, 2017

kball Jan 19, 2017

ncoden Jan 18, 2017

kball Jan 19, 2017

ncoden Jan 19, 2017 •

edited

Loading

ncoden Mar 22, 2018

DanielRuf Mar 23, 2018

ncoden Mar 23, 2018

ncoden Jan 18, 2017

kball Jan 19, 2017

ahebrank commented Jan 19, 2017

kball commented Jan 19, 2017

ahebrank commented Jan 19, 2017

kball commented Jan 19, 2017

designerno1 commented Jan 19, 2017

kball commented Jan 20, 2017

designerno1 commented Jan 20, 2017

ahebrank Jan 21, 2017 •

edited

Loading

ahebrank Jan 21, 2017 •

edited

Loading

ahebrank commented Jan 21, 2017

		@@ -297,7 +304,7 @@ class Tabs {
		* @param {jQuery \| String} elem - jQuery object or string of the id of the pane to display.
		* @function

Fix tabs deeplink history to work with back button within page #9674

Fix tabs deeplink history to work with back button within page #9674

Conversation

kball commented Jan 18, 2017

kball commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncoden Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahebrank commented Jan 19, 2017

kball commented Jan 19, 2017

ahebrank commented Jan 19, 2017

kball commented Jan 19, 2017

designerno1 commented Jan 19, 2017

kball commented Jan 20, 2017

designerno1 commented Jan 20, 2017

ahebrank Jan 21, 2017 • edited Loading

Choose a reason for hiding this comment

ahebrank Jan 21, 2017 • edited Loading

Choose a reason for hiding this comment

ahebrank commented Jan 21, 2017

ncoden Jan 19, 2017 •

edited

Loading

ahebrank Jan 21, 2017 •

edited

Loading

ahebrank Jan 21, 2017 •

edited

Loading