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

✨ amp-sidebar: support autoscroll #20524

Merged
merged 9 commits into from Feb 5, 2019
Merged

Conversation

aghassemi
Copy link
Contributor

@aghassemi aghassemi commented Jan 25, 2019

Closes #19570

amp-sidebar can automatically scroll the overflowing container to first element that is decorated with autoscroll an attribute in both sidebar and toolbar cases.

This feature is useful when dealing with long navigation list and wanting the sidebar to scroll to the current navigation items when page loads.

When using toolbar feature, autoscroll only works if the <nav toolbar> element is set to overflow: auto or overflow: scroll.

Demo

https://aghassemi-amphtml.firebaseapp.com/examples/amp-sidebar-autoscroll.amp.html

2

a

Snippet

Responsive (desktop and mobile)

<style amp-custom="">

  nav [toolbar] {
    overflow: auto;
  }

</style>

<amp-sidebar id="sidebar1" layout="nodisplay" side="right">
  <nav toolbar="(max-width: 767px)" toolbar-target="target-element">
    <ul>
      <li>Nav item 1</li>
      <li>Nav item 2</li>
      <li>Nav item 3</li>
      <li autoscroll class="currentPage">Nav item 4</li>
      <li>Nav item 5</li>
      <li>Nav item 6</li>
    </ul>
  </nav>
</amp-sidebar>

<div id="target-element">
</div>

Mobile only

<amp-sidebar id="sidebar1" layout="nodisplay" side="right">
    <ul>
      <li>Nav item 1</li>
      <li>Nav item 2</li>
      <li>Nav item 3</li>
      <li autoscroll class="currentPage">Nav item 4</li>
      <li>Nav item 5</li>
      <li>Nav item 6</li>
    </ul>
</amp-sidebar>

Full Example:

https://github.com/ampproject/amphtml/blob/2ed7c73b405fff323400aee331c32602945bebb6/examples/amp-sidebar-autoscroll.amp.html

/To @alanorozco @CrystalFaith
/FYI @pbakaus @nainar

@ampproject ampproject deleted a comment Feb 3, 2019
@ampproject ampproject deleted a comment Feb 3, 2019
@aghassemi aghassemi changed the title ✨ amp-sidebar: support autoscroll (WIP) ✨ amp-sidebar: support autoscroll Feb 3, 2019
extensions/amp-sidebar/0.1/autoscroll.js Outdated Show resolved Hide resolved
extensions/amp-sidebar/amp-sidebar.md Outdated Show resolved Hide resolved
@aghassemi
Copy link
Contributor Author

@CrystalOnScript merging to make the Canary cut, will implement any changes you like to see in the next PR.

@honeybadgerdontcare I am gonna go with assumption this is very likely safe and merge to make the Canary cut, let me know if you find anything later and I'll deal with it.

Thanks!

@aghassemi aghassemi merged commit 53e533b into ampproject:master Feb 5, 2019
@honeybadgerdontcare
Copy link
Contributor

@aghassemi okay, I'll skip testing it then

@Gregable
Copy link
Member

Gregable commented Feb 5, 2019

This change breaks a small number of documents which have an <amp-selector> descendant of <amp-sidebar>. The problem arises because two reference points end up applying to one tag, since <amp-selector> also defines reference points.

I suggest we revert this change for now, and then reconsider how to move it forward again.

@Gregable
Copy link
Member

Gregable commented Feb 5, 2019

@aghassemi

@aghassemi
Copy link
Contributor Author

@newmuis Jon, we are working on validation rules for this. Do you want to allow this (essentially the second gif in the description) in AMP Story or should we prevent it?

@newmuis
Copy link
Contributor

newmuis commented Feb 5, 2019

It can be allowed, since (AFAICT) it does not have any effect on the document outside of the sidebar.

/cc @bramanudom who owns the <amp-sidebar> integration in stories.

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

Successfully merging this pull request may close these issues.

None yet

7 participants