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

Projects
None yet
6 participants
@ahebrank
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@kball

kball Oct 7, 2016

Collaborator

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.

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@ahebrank

ahebrank Oct 8, 2016

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@kball

kball Oct 25, 2016

Collaborator

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?

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@Owlbertz

Owlbertz Oct 26, 2016

Collaborator

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.)

Collaborator

Owlbertz commented Oct 26, 2016

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

This comment has been minimized.

Show comment
Hide comment
@coreysyms

coreysyms Oct 26, 2016

Collaborator

@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.

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@coreysyms

coreysyms Oct 27, 2016

Collaborator

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;
}
Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@coreysyms

coreysyms Oct 27, 2016

Collaborator

sorry wrong button, reopened

Collaborator

coreysyms commented Oct 27, 2016

sorry wrong button, reopened

@ahebrank

This comment has been minimized.

Show comment
Hide comment
@ahebrank

ahebrank Oct 29, 2016

Contributor

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);
    }
Contributor

ahebrank commented Oct 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@kball

kball Oct 31, 2016

Collaborator

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?

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@coreysyms

coreysyms Oct 31, 2016

Collaborator

@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

Collaborator

coreysyms commented Oct 31, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@ahebrank

ahebrank Nov 4, 2016

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@coreysyms

coreysyms Nov 7, 2016

Collaborator

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

Collaborator

coreysyms commented Nov 7, 2016

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

@kball

This comment has been minimized.

Show comment
Hide comment
@kball

kball Nov 8, 2016

Collaborator

Sweeeeet! Great work @ahebrank

Collaborator

kball commented Nov 8, 2016

Sweeeeet! Great work @ahebrank

@kball kball merged commit c092819 into zurb:v6.3 Nov 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jaysonbrown

This comment has been minimized.

Show comment
Hide comment
@jaysonbrown

jaysonbrown Nov 8, 2016

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

jaysonbrown commented Nov 8, 2016

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

@kball

This comment has been minimized.

Show comment
Hide comment
@kball

kball Nov 8, 2016

Collaborator

@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.

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@jaysonbrown

jaysonbrown Nov 8, 2016

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

jaysonbrown commented Nov 8, 2016

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

@RLaptev

This comment has been minimized.

Show comment
Hide comment
@RLaptev

RLaptev 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)….

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

This comment has been minimized.

Show comment
Hide comment
@ahebrank

ahebrank Nov 9, 2016

Contributor

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

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

ahebrank commented Nov 9, 2016

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

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

This comment has been minimized.

Show comment
Hide comment
@RLaptev

RLaptev Nov 9, 2016

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

RLaptev commented Nov 9, 2016

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

@kball

This comment has been minimized.

Show comment
Hide comment
@kball

kball Nov 9, 2016

Collaborator

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

Collaborator

kball commented Nov 9, 2016

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

@jaysonbrown

This comment has been minimized.

Show comment
Hide comment

jaysonbrown commented Nov 9, 2016

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