No previous from tap on card on desktop #12671

prateekbh · 2018-01-04T23:55:51Z

This makes navigation only go forward, when clicked anywhere on card while on desktop.

alanorozco · 2018-01-09T23:35:53Z

extensions/amp-story/0.1/page-advancement.js

@@ -286,7 +287,8 @@ class ManualAdvancement extends AdvancementConfig {
        ((1 - NEXT_SCREEN_AREA_RATIO) * offsetWidth);
    const nextScreenAreaMax = offsetLeft + offsetWidth;

-    if (event.pageX >= nextScreenAreaMin && event.pageX < nextScreenAreaMax) {
+    if ((event.pageX >= nextScreenAreaMin && event.pageX < nextScreenAreaMax)


Adding knowledge to ManualAdvancement about desktop mode tangles things, IMO.

How about reversing the logic flow? ManualAdvancement could pass along that it comes from an areaClick or something like that, and then AmpStory can reject it.

// ManualAdvancement if (event.pageX >= nextScreenAreaMin && event.pageX < nextScreenAreaMax) { this.onAdvance(/* opt_isFromAreaClick */ true); }

// AmpStory buildCallback() { //... this.advancement_.addAdvanceListener(isFromAreaClick => { // TODO(alanorozco): opt_isAutomatic is incorrect in this case, should be passed from // advancement class. Ideally refactor into AdvancementType enum (`AUTO/AREA_CLICK/BUTTON_CLICK`). this.next(/* opt_isAutomaticAdvance */ !isFromAreaClick, isFromAreaClick)); }); } // next() passes arg thru to switchTo_() // ... switchTo_(targetPageId, opt_isFromAreaClick) { const payload = {targetPageId, isFromAreaClick: !!opt_isFromAreaClick}; // ... }

@alanorozco I completely agree with this.

However in the above approach i'll still be able to either let it go forward or ignore it.
There would still be a probability that I end up calling previous from ManualAdvancement and in the end call .next from inside it. which would again be ugly.

Can we instead remove onAdvance / onPrevious from AdvancementConfiguration and just have onNavigate? This ways AdvancementConfiguration and its subclasses can pass the type of navigation and its metadata.

e.g. onNavigate({NAVIGATION.MANUAL/TIMER/MEDIA, TAP_AREA: 0.8}) and then AMP-STORY-PAGE can decide whether to call .next or .previous

As discussed offline: Best idea is to add a new NAVIGATION event that specifies direction and both target IDs.

…oBackOnCardOnDesktop

alanorozco · 2018-01-11T19:50:38Z

extensions/amp-story/0.1/amp-story-page.js

-    if (pageId === null) {
-      dispatch(this.element, EventType.SHOW_BOOKEND, /* opt_bubbles */ true);
-      return;
+    if (navigationDirection === NAVIGATION_DIRECTION.NEXT) {


The approach can be changed so that concepts aren't as scattered, and won't require you to add a new concept for tap/button/keyboard navigation.

Other bonuses of the proposed approach:

Improves readability.

Isolates logical concepts.

Fewer changes.

Paves the way for:

Further simplification

Fixing the incorrect automatic advance bug (which also allows me to make a change to pass it to analytics! yay!).

Split concepts early on

Advancement.addPreviousListener and Advancement.addNextListener should remain as-is.

Create an additional Advancement.addTapNavigationListener method. This will be defined at the super-class level (AdvancementConfig): add onTapNavigation() method (same implementation as onAdvance() and onPrevious()) and trigger that from maybePerformNavigation_()

Pass a TapNavigationDirection enum to tapNavigationListener callbacks.

Note: TapNavigationDirection must be defined as capitalized camel-case (per our style) and annotated as an enum:

/** @const @enum */ export const TapNavigationDirection = { NEXT: 1, PREVIOUS: 2, };

Since this leaves the concept of next/previous intact, you can revert the changes made to AmpStory and AmpStoryPage.

Create a new event type that will work as an "adapter".

Dispatch event from the AmpStoryPage level.

onTapNavigation_(direction) { // @param {!TapNavigationDirection} direction dispatchCustom(this.win, this.element, EventType.TAP_NAVIGATION, { direction, previousPageId: this.getPreviousPageId_(), nextPageId: this.getNextPageId_(), }); }

Let AmpStory convert the event.

This way we contain the special-case for desktop in only one callback!

Note that dispatchNextOrBookend_ and dispatchPreviousOrHint_ trigger on this.element (== <amp-story>).

this.element.addEventListener(EventType.TAP_NAVIGATION, e => { const {direction, nextPageId, previousPageId} = e.details; if (this.isDesktop_()) { // Desktop should always go to next page on page tap. this.dispatchNextOrBookend_(nextPageId); return; } if (direction === TapNavigationDirection.NEXT) { this.dispatchNextOrBookend_(previousPageId); return; } if (direction === TapNavigationDirection.PREVIOUS) { this.dispatchPreviousOrHint_(previousPageId); return; } });

alanorozco · 2018-01-11T23:44:19Z

extensions/amp-story/0.1/amp-story-page.js

+   * Delegated the navigation decision to AMP-STORY via event.
+   * @param {number} navigationDirection The direction in which navigation needs to takes place.
+   */
+  navigate(navigationDirection) {


navigateOnTap(direction) {

alanorozco · 2018-01-11T23:45:06Z

extensions/amp-story/0.1/page-advancement.js

@@ -28,6 +28,11 @@ const NEXT_SCREEN_AREA_RATIO = 0.75;
 /** @const {number} */
 const POLL_INTERVAL_MS = 250;

+/** @const {!Object<string, number>} */


/** @const @enum */ export const TapNavigationDirection { NEXT: 1, PREVIOUS: 2, };

alanorozco · 2018-01-11T23:48:04Z

extensions/amp-story/0.1/amp-story.js

@@ -354,6 +354,18 @@ export class AmpStory extends AMP.BaseElement {
      this.ampStoryHint_.showFirstPageHintOverlay();
    });

+    this.element.addEventListener(EventType.TAP_NAVIGATION, e => {
+      const {navigationDirection} = e.detail;
+      if (navigationDirection === NAVIGATION_DIRECTION.NEXT) {


Remove navigation as a prefix (i.e. direction is fine)

Short-circuit classes for special cases are always better.

if (this.isDesktop_()) { this.next_(); return; } if (direction == TapNavigationDirection.PREVIOUS) { this.previous_(); } else { this.next_(): }

just curious why is one better that other?
i know the care where the else is huge block and gets intended, but why otherwise?

Yeah, it's more readable as you mentioned. It also makes a clearer change if it ever gets removed in the future.

alanorozco · 2018-01-11T23:48:24Z

extensions/amp-story/0.1/amp-story-page.js

+   */
+  navigate(navigationDirection) {
+    // const prevPageId = this.getPreviousPageId_();
+    // const nextPageId = this.getNextPageId_(/* opt_isAutomaticAdvance */ true);


Remove commented-out code.

alanorozco · 2018-01-12T00:25:49Z

extensions/amp-story/0.1/page-advancement.js

+   * @param {number} navigationDirection Direction of navigation
+   * @protected
+   */
+  onNavigate(navigationDirection) {


onTapNavigation

…oBackOnCardOnDesktop

alanorozco · 2018-01-12T16:11:44Z

extensions/amp-story/0.1/amp-story-page.js

+   * Delegated the navigation decision to AMP-STORY via event.
+   * @param {number} direction The direction in which navigation needs to takes place.
+   */
+  navigate(direction) {


nit: navigateOnTap

alanorozco · 2018-01-12T16:12:15Z

extensions/amp-story/0.1/page-advancement.js

+   * @param {number} navigationDirection Direction of navigation
+   * @protected
+   */
+  onNavigate(navigationDirection) {


newmuis · 2018-01-12T17:47:03Z

extensions/amp-story/0.1/amp-story.js

+
+      if (direction === TapNavigationDirection.NEXT) {
+        this.next_();
+      } else {


nit: can you explicitly check that this is TapNavigationDirection.PREVIOUS?

* no back from card tap on desktop * fixing types * refactor manual advancement * changing names * explicit previous check

no back from card tap on desktop

73ce974

prateekbh requested review from alanorozco and dreamofabear January 4, 2018 23:55

googlebot added the cla: yes label Jan 4, 2018

prateekbh added 2 commits January 5, 2018 11:40

Merge branch 'master' into noBackOnCardOnDesktop

b691356

fixing types

133d5c9

alanorozco requested changes Jan 9, 2018

View reviewed changes

Merge branch 'master' of https://github.com/ampproject/amphtml into n…

c7774b5

…oBackOnCardOnDesktop

prateekbh changed the title ~~No previous from tap on card on desktop~~ [WIP] No previous from tap on card on desktop Jan 11, 2018

prateekbh changed the title ~~[WIP] No previous from tap on card on desktop~~ No previous from tap on card on desktop Jan 11, 2018

alanorozco requested changes Jan 11, 2018

View reviewed changes

refactor manual advancement

4584157

alanorozco approved these changes Jan 12, 2018

View reviewed changes

Merge branch 'master' of https://github.com/ampproject/amphtml into n…

a9db922

…oBackOnCardOnDesktop

alanorozco approved these changes Jan 12, 2018

View reviewed changes

changing names

c6df609

newmuis reviewed Jan 12, 2018

View reviewed changes

explicit previous check

9245e5c

newmuis merged commit ff974c1 into ampproject:master Jan 12, 2018

prateekbh deleted the noBackOnCardOnDesktop branch January 12, 2018 19:39

prateekbh mentioned this pull request Jan 16, 2018

add on tap listener for multiple advance #12844

Merged

gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018

No previous from tap on card on desktop (ampproject#12671)

9a1939f

* no back from card tap on desktop * fixing types * refactor manual advancement * changing names * explicit previous check

RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018

No previous from tap on card on desktop (ampproject#12671)

22ec7a7

* no back from card tap on desktop * fixing types * refactor manual advancement * changing names * explicit previous check

ampprojectbot mentioned this pull request Aug 17, 2022

🚨 Error: Cannot read properties of null (reading 'next') ampproject/error-reporting#164

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No previous from tap on card on desktop #12671

No previous from tap on card on desktop #12671

prateekbh commented Jan 4, 2018

alanorozco Jan 9, 2018

prateekbh Jan 10, 2018

alanorozco Jan 10, 2018

alanorozco Jan 11, 2018 •

edited

Loading

alanorozco Jan 11, 2018

prateekbh Jan 12, 2018

alanorozco Jan 11, 2018

prateekbh Jan 12, 2018

alanorozco Jan 11, 2018

prateekbh Jan 12, 2018

prateekbh Jan 12, 2018

alanorozco Jan 12, 2018

alanorozco Jan 11, 2018

prateekbh Jan 12, 2018

alanorozco Jan 12, 2018

alanorozco Jan 12, 2018

prateekbh Jan 12, 2018

alanorozco Jan 12, 2018

prateekbh Jan 12, 2018

alanorozco Jan 12, 2018

newmuis Jan 12, 2018

prateekbh Jan 12, 2018

No previous from tap on card on desktop #12671

No previous from tap on card on desktop #12671

Conversation

prateekbh commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanorozco Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Split concepts early on

Create a new event type that will work as an "adapter".

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

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

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

alanorozco Jan 11, 2018 •

edited

Loading