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

Fix content smooth scrolling in Twenty Seventeen #1777

Merged
merged 1 commit into from Jan 17, 2019

Conversation

Projects
None yet
3 participants
@westonruter
Copy link
Member

westonruter commented Dec 23, 2018

In the Twenty Seventeen theme of WordPress, there is a down arrow on the homepage that upon clicking causes a smooth scroll down to the content area:

image

This arrow appears in markup as a link:

<a href="#content" class="menu-scroll-down">...</a>

The theme implements the smooth-scroll behavior via the jQuery scrollTo plugin, with a page navigation jump for browsers that have JS disabled.

In the AMP version of the theme, however, we omit that plugin since it is not valid AMP, and the AMP plugin then instead adds an on attribute to the element:

<a href="#content" on="tap:content.scrollTo(duration=600)" class="menu-scroll-down">...</a>

I believe this did work at one time, but it is not working any longer. Instead of there being a smooth scroll behavior, the browser now just jumps to the #content element in the page.

Since there is no way in AMP currently to call event.preventDefault(), I saw via ampproject/amphtml#8810 fix seems to be to just remove the href while also add a role=button and tabindex=0. This ensures that the smooth scroll behavior happens for both clicks and keypresses:

<a on="tap:content.scrollTo(duration=600)" role="button" tabindex="0" class="menu-scroll-down">...</a>

The downside here is that the link will no longer work with JS turned off, but that will be the least of the problems on the page if that is the case. It would be better if there was a preventDefault action so you could do:

<a href="#content" on="tap:content.scrollTo(duration=600),preventDefault" class="menu-scroll-down">...</a>

But this is not currently supported in AMP.

@westonruter westonruter force-pushed the fix/twentyseventeen-smooth-scrolling branch from fbad4b6 to eac1fbb Dec 23, 2018

@westonruter westonruter requested review from swissspidy and removed request for amedina Jan 17, 2019

@swissspidy
Copy link
Collaborator

swissspidy left a comment

Works like a charm.

I had some reservations at first because of accessibility when JS is disabled, but somehow it seems to work fine.

@westonruter westonruter merged commit a790678 into 1.0 Jan 17, 2019

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the fix/twentyseventeen-smooth-scrolling branch Jan 17, 2019

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Jan 18, 2019

I found a tiny issue: the down-arrow doesn't have a pointer cursor.

westonruter added a commit that referenced this pull request Jan 18, 2019

westonruter added a commit that referenced this pull request Jan 18, 2019

Merge branch '1.0' of github.com:ampproject/amp-wp
* '1.0' of github.com:ampproject/amp-wp: (29 commits)
  Fix readme date and typo
  Update readme with late 1.0.2 changes
  Restore cursor:pointer for menu-scroll-down after #1777
  Move wp-i18n and wp-dom-ready into src directory
  Eliminate needless building of amp-validation-tooltips.js
  Exclude assets/src from build
  Bump version to 1.0.2 and add changelog entry
  Update welcome prompt to point to Getting Started section on amp-wp.org
  Restrict assets to only be enqueued on amp_validated_url edit post screen
  Remove erroneous call to wp() in AMP_Widget_Text::inject_video_max_width_style()
  Add _doing_it_wrong() call when is_amp_endpoint() is called before wp action
  Remove unnecessary condition around polyfilling wp-i18n and wp-dom-ready
  Remove WP-CLI dependency for build
  Remove obsolete languages directory
  Make get_validated_url_file_path() better compatible with register_theme_directory()
  Add WP-CLI i18n command as explicit Composer dev dependency
  Improve performance of make-pot command by excluding more paths
  Add missing translators comment in amp-block-validation.js
  Polyfill wp-i18n and wp-dom-ready for non-Gutenberg 4.9 installs
  Re-install WP dom-ready & i18n as devDependencies
  ...

westonruter added a commit that referenced this pull request Jan 18, 2019

Merge tag '1.0.2' of github.com:ampproject/amp-wp into update/develop…
…-to-1.0.2

* tag '1.0.2' of github.com:ampproject/amp-wp:
  Fix readme date and typo
  Update readme with late 1.0.2 changes
  Restore cursor:pointer for menu-scroll-down after #1777
  Move wp-i18n and wp-dom-ready into src directory
  Eliminate needless building of amp-validation-tooltips.js
  Exclude assets/src from build
  Bump version to 1.0.2 and add changelog entry
  Update welcome prompt to point to Getting Started section on amp-wp.org
  Fix content smooth scroll in Twenty Seventeen
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.