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

No previous from tap on card on desktop #12671

Merged
merged 8 commits into from
Jan 12, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion extensions/amp-story/0.1/amp-story-page.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ export class AmpStoryPage extends AMP.BaseElement {
this.advancement_.addPreviousListener(() => this.previous());
this.advancement_
.addAdvanceListener(() => this.next(/* opt_isAutomaticAdvance */ true));
this.advancement_.addOnTapNavigationListener(
navigationDirection => this.navigate(navigationDirection));
this.advancement_
.addProgressListener(progress => this.emitProgress_(progress));
}
Expand Down Expand Up @@ -404,7 +406,6 @@ export class AmpStoryPage extends AMP.BaseElement {
return this.element.hasAttribute('active');
}


/**
* Emits an event indicating that the progress of the current page has changed
* to the specified value.
Expand Down Expand Up @@ -524,6 +525,17 @@ export class AmpStoryPage extends AMP.BaseElement {
this.switchTo_(pageId);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: navigateOnTap

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

const payload = {direction};
const eventInit = {bubbles: true};
dispatchCustom(this.win, this.element, EventType.TAP_NAVIGATION, payload,
eventInit);
}


/**
* @param {string} targetPageId
Expand Down
17 changes: 16 additions & 1 deletion extensions/amp-story/0.1/amp-story.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ import {dict} from '../../../src/utils/object';
import {renderSimpleTemplate} from './simple-template';
import {MediaPool, MediaType} from './media-pool';
import {PaginationButtons} from './pagination-buttons';

import {TapNavigationDirection} from './page-advancement';


/** @private @const {string} */
Expand Down Expand Up @@ -345,6 +345,21 @@ export class AmpStory extends AMP.BaseElement {
this.ampStoryHint_.showFirstPageHintOverlay();
});

this.element.addEventListener(EventType.TAP_NAVIGATION, e => {
const {direction} = e.detail;

if (this.isDesktop_()) {
this.next_();
return;
}

if (direction === TapNavigationDirection.NEXT) {
this.next_();
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

this.previous_();
}
});

const gestures = Gestures.get(this.element,
/* shouldNotPreventDefault */ true);

Expand Down
3 changes: 3 additions & 0 deletions extensions/amp-story/0.1/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ export const EventType = {
// Triggered when the story should switch to the next page
NEXT_PAGE: 'ampstory:nextpage',

// Triggered when the story should navigate after a tap on active page.
TAP_NAVIGATION: 'ampstory:tapnavigation',

// Triggered when a page updates its progress
PAGE_PROGRESS: 'ampstory:pageprogress',

Expand Down
30 changes: 28 additions & 2 deletions extensions/amp-story/0.1/page-advancement.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ const NEXT_SCREEN_AREA_RATIO = 0.75;
/** @const {number} */
const POLL_INTERVAL_MS = 250;

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

/**
* Base class for the AdvancementConfig. By default, does nothing other than
Expand All @@ -45,6 +50,9 @@ export class AdvancementConfig {
/** @private @const {!Array<function()>} */
this.previousListeners_ = [];

/** @private @const {!Array<function(number)>} */
this.tapNavigationListeners_ = [];

/** @private {boolean} */
this.isRunning_ = false;
}
Expand Down Expand Up @@ -75,6 +83,14 @@ export class AdvancementConfig {
this.previousListeners_.push(previousListener);
}

/**
* @param {function(number)} onTapNavigationListener A function that handles when a
* navigation listener to be fired.
*/
addOnTapNavigationListener(onTapNavigationListener) {
this.tapNavigationListeners_.push(onTapNavigationListener);
}

/**
* Invoked when the advancement configuration should begin taking effect.
*/
Expand Down Expand Up @@ -127,6 +143,16 @@ export class AdvancementConfig {
});
}

/**
* @param {number} navigationDirection Direction of navigation
* @protected
*/
onNavigate(navigationDirection) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onTapNavigation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

this.tapNavigationListeners_.forEach(navigationListener => {
navigationListener(navigationDirection);
});
}

/**
* Provides an AdvancementConfig object for the specified amp-story-page.
* @param {!./amp-story-page.AmpStoryPage} page
Expand Down Expand Up @@ -287,9 +313,9 @@ class ManualAdvancement extends AdvancementConfig {
const nextScreenAreaMax = offsetLeft + offsetWidth;

if (event.pageX >= nextScreenAreaMin && event.pageX < nextScreenAreaMax) {
this.onAdvance();
this.onNavigate(TapNavigationDirection.NEXT);
} else if (event.pageX >= offsetLeft && event.pageX < nextScreenAreaMin) {
this.onPrevious();
this.onNavigate(TapNavigationDirection.PREVIOUS);
}
}
}
Expand Down