Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

addresses #7652: deep linking for tabs and browser history update #9242

Merged
merged 4 commits into from
Nov 8, 2016

Conversation

ahebrank
Copy link
Contributor

@ahebrank ahebrank commented Oct 7, 2016

First attempt to include workaround from #7652 (comment)

This is currently totally untested and I haven't even added it into an existing project to see if it works at all.

Adds 4 new options to tabs:

  /**
   * Allows the window to scroll to content of pane specified by hash anchor
   * @option
   * @example false
   */
  deepLink: false,

  /**
   * Adjust the deep link scroll to make sure the top of the tab panel is visible
   * @option
   * @example false
   */
  deepLinkSmudge: true,

  /**
   * Animation time (ms) for the deep link adjustment
   * @option
   * @example 300
   */
  deepLinkSmudgeDelay: 300,

  /**
   * Update the browser history with the open tab
   * @option
   * @example false
   */
  updateHistory: true,

@kball
Copy link
Contributor

kball commented Oct 7, 2016

Awesome! We're in the middle of QA for 6.2.4 so it will be a bit before we can test this out but this will be great for 6.3.

@ahebrank
Copy link
Contributor Author

ahebrank commented Oct 8, 2016

OK, now tested in practice and fixed a little--should be working with the default options as I have them and

<ul class="tabs" data-tabs data-deep-link="true" id="example-tabs">

to turn on deep linking.

@kball
Copy link
Contributor

kball commented Oct 25, 2016

I like this... @Owlbertz @colin-marshall @coreysyms I'm thinking this approach could form the foundation of deep linking across various other components as well (tabs, accordion). What do you think?

@Owlbertz
Copy link
Contributor

Sounds like a good idea in general for me. Not sure if this is already in place, but other components I think of are Magellan and maybe even Orbit and Reveal. (I am a huge fan of being able to send links that reflect exactly the state of the website that I am currently seeing.)

@coreysyms
Copy link
Contributor

coreysyms commented Oct 26, 2016

@kball @Owlbertz @colin-marshall I like this PR however I have some notes / changes:

  1. I don't like that updateHistory is default true, this should be false by default. Annoying the user clicking around tabs and then trying to figure out the back button should be opt-in only IMO.
  2. Perhaps the default functionality for updateHistory should be replaceState() (not affecting the history but appending the address bar) and updateHistory:true kicks over to pushState() (affecting the history in addition to the address bar)
  3. I really don't like adding in js animations. I wouldn't want my framework manipulating my page in this manner even with the best of intentions.
  4. Perhaps a solution for, as @ahebrank puts it, "smudging" would be CSS and utilizing :target to "smudge" the title into view. We are moving towards address bar appendage of the hash via history manipulation anyway. This would remove the need for the `smudge' functionality and options.

Open for thoughts and discussion.

@coreysyms
Copy link
Contributor

coreysyms commented Oct 27, 2016

Before I forget to suggest code for "smudging" here is a snippet that could work as a jump off point. When the address bar hits #panel1 the top of tabs box is "smudged" down. I'm not the best CSS person in the world, but someone may know a more graceful way.

.tabs-panel:target { 
  display: block; 
  margin-top: -3.25rem;
  padding-top:4.25rem;
}

@coreysyms coreysyms closed this Oct 27, 2016
@coreysyms coreysyms reopened this Oct 27, 2016
@coreysyms
Copy link
Contributor

sorry wrong button, reopened

@ahebrank
Copy link
Contributor Author

I won't claim there isn't one, but I can't think of a way to safely bring the tab titles into the viewport without calculating the offset after page load. Positioning the :target pulls up the tab content background up behind the title bar, which breaks my test example. It's also pretty hard to figure out the exact offset statically (although you can get a good guess with something like $tabs-target-offset: 1rem + (nth($tab-item-padding, 1) * 2)).

I'm not hung up on this smudge/offset behavior since it's easy to do outside the framework, but we almost always use it to make the tab titles visible.

I'm good with the other suggestion:

    //either replace or update browser history
    var anchor = $target.find('a').attr('href');
    if (this.options.updateHistory) {
      history.pushState({}, "", anchor);
    } else {
      history.replaceState({}, "", anchor);
    }

@kball
Copy link
Contributor

kball commented Oct 31, 2016

Given the variability of styles on tabs I don't know a great way to default it into view... here would be my suggestion:

  1. Default the smudge behavior to false so that by default we're not messing with folks' default behavior.
  2. Add some documentation on how to do this by CSS using :target
  3. Add an event when we trigger this deeplink behavior so someone wanting to do their own 'smudge' style behavior can do so.

I also do think the updateHistory option is a must.

@coreysyms @ahebrank what do you think?

@coreysyms
Copy link
Contributor

@kball This will work for me.
@ahebrank Could you please:

  1. Add in the snippet you have on history
  2. Make sure updateHistory is false on default
  3. Make sure deepLinkSmudge is also false on default, but leave in your "smudging" animation.

Thanks for this PR, after these changes come in, we should push it to 6.3

@ahebrank
Copy link
Contributor Author

ahebrank commented Nov 4, 2016

Points from @coreysyms should now be addressed. Also added some documentation and a deeplink event as suggested by @kball.

@coreysyms
Copy link
Contributor

@kball this looks good to me, take a look when you get a sec.

@kball
Copy link
Contributor

kball commented Nov 8, 2016

Sweeeeet! Great work @ahebrank

@kball kball merged commit c092819 into foundation:v6.3 Nov 8, 2016
@jaysonbrown
Copy link

Is there also the API call back, for example: current tab is #4 ?

@kball
Copy link
Contributor

kball commented Nov 8, 2016

@jaysonbrown there are events triggered on deeplink (deeplink.zf.tabs) and on tab change (change.zf.tabs) that you can listen to in order to detect changes.

@jaysonbrown
Copy link

@kball Thanks for the reply, that's perfect!

@RLaptev
Copy link

RLaptev commented Nov 9, 2016

@kball
When page is loaded with tab specified, e.g. page#tab2, I'm getting this.$element is not defined error on line 96 this.$element.trigger('deeplink.zf.tabs', [$target]);
Changing it to _this.$element fixes the error, but then a new error appears $target is not defined (same line)….

@ahebrank
Copy link
Contributor Author

ahebrank commented Nov 9, 2016

@RLaptev, that was dumb on my part. Does this fix it?

$elem.trigger('deeplink.zf.tabs', [$(anchor)]);

@RLaptev
Copy link

RLaptev commented Nov 9, 2016

@ahebrank
Yes! The above code fixes the issue. Thanks!

@kball
Copy link
Contributor

kball commented Nov 9, 2016

@ahebrank would you like to submit a PR with this fix?

@jaysonbrown
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants