Adds support for RTL navigation in amp-story pages ✨ #16381

Enriqe · 2018-06-26T18:05:51Z

Flips the navigational click areas in an amp-story page when dir="rtl" is used in the document.

This means the next screen area that is positioned on the right for LTR languages will be on the left, and the previous screen area will be on the right. Same sizes of these areas will be taken into account.

A follow up PR will address the education overlay so that it matches these areas in RTL.

newmuis

Overall, this leaves a lot of duplicated code. Is there some way where we can, for example, control the ratio between the two regions and also the action of each of the two regions?

For example:

const sections = {
  left: {
    width: isLtr ? 1/4 : 3/4,
    direction: isLtr ? TapNavigationDirection.PREVIOUS : TapNavigationDirection.NEXT,
  },
  right: {
    width: isLtr ? 3/4 : 1/4,
    direction: isLtr ? TapNavigationDirection.NEXT : TapNavigationDirection.PREVIOUS,
  },
};
const tapDirection = this.getTapDirection(clickPositionX, sections);

@Enriqe, @gmajoulet WDYT?

newmuis · 2018-06-26T18:14:47Z

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

+  /**
+   * Checks if click is inside the previous screen area.
+   * @param {number} clickPositionX
+   * @param {DOMRect} pageRect


Explicitly use ? for nullable or ! for non-nullable.

gmajoulet · 2018-06-26T18:44:20Z

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

+    let previousScreenAreaMin;
+    let previousScreenAreaMax;
+
+    if (isRTL(this.element_.ownerDocument)) {


Eventually you'll want to have a RTL_STATE in the store, so you can avoid duplicating this. It'll also make it easier to propagate the dir="rtl" attributes in the shadow roots.

gmajoulet · 2018-06-26T18:45:27Z

+1 to @newmuis comments, that structure should allow you to make the code a lot easier to read.

Enriqe · 2018-06-27T22:26:00Z

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

+          TapNavigationDirection.NEXT : TapNavigationDirection.PREVIOUS,
+      },
+      right: {
+        width: isRtl ? smallArea : bigArea,


For the moment this is not used, but thought i'd leave it in case we need it in the future.

gmajoulet

A few nits

gmajoulet · 2018-06-27T22:27:49Z

extensions/amp-story/1.0/amp-story-store-service.js

@@ -64,6 +65,7 @@ export const StateProperty = {
  DESKTOP_STATE: 'desktopstate',
  HAS_AUDIO_STATE: 'hasaudiostate',
  INFO_DIALOG_STATE: 'infodialogstate',
+  IS_RTL_STATE: 'isrtlstate',


Nit: RTL_STATE would be shorter and provide the same information

gmajoulet · 2018-06-27T22:29:06Z

extensions/amp-story/1.0/amp-story.js

@@ -580,6 +581,12 @@ export class AmpStory extends AMP.BaseElement {

    this.initializeBookend_();

+    const {document} = this.win;


Nit: you don't need this extra const, please use directly this.win.document

You're right, I thought we preferred destructuring object like in #15204, but it's not the case here.

gmajoulet · 2018-06-27T22:34:14Z

extensions/amp-story/1.0/amp-story.js

+    const {document} = this.win;
+
+    if (isRTL(document)) {
+      this.storeService_.dispatch(Action.TOGGLE_RTL, true);


I think we should try to set this state as soon as possible in the code execution, like we do for desktop.

Right now, the RTL state is set after some components are initialized, which means you'd have to watch for RTL_STATE updates from every component, to later set the dir="rtl" attributes.
This can be avoided if you can find a way to instantiate the state right away with the proper value, or set it first thing in the buildCallback of amp-story.

gmajoulet · 2018-06-27T22:39:36Z

extensions/amp-story/1.0/amp-story.js

-        this.next_();
-        break;
+    const keyDirection = e.keyCode;
+    const isRtl = this.storeService_.get(StateProperty.IS_RTL_STATE);


Nit: to further avoid confusions between isRTL and isRtl, maybe RTLState?

Technically, I think it's supposed to be Rtl per the style guide (not that we strictly follow the style guide), since only the first letter of acronyms get capitalized.

Per styleguide I will stick to RtlState

gmajoulet · 2018-06-27T22:45:30Z

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

+    const smallArea = (1 - NEXT_SCREEN_AREA_RATIO) * offsetWidth;
+    const isRtl = this.storeService_.get(StateProperty.IS_RTL_STATE);
+
+    const sections = {


Is there any way we can avoid creating a new instance of this object on every user interaction?

Maybe we could store the width as a ratio, and then do the operation in the getTapDirection_ method.
ie:

left: { width: RTLState ? 0.75 : 0.25, direction: ..., }

gmajoulet · 2018-06-27T22:49:12Z

extensions/amp-story/1.0/amp-story.js

+      this.next_();
+    } else if ((keyDirection == KeyCodes.RIGHT_ARROW && isRtl) ||
+        (keyDirection == KeyCodes.LEFT_ARROW && !isRtl)) {
+      this.previous_();


Maybe the original version would be easier to read:

const RTLState = this.storeService_.get(StateProperty.RTL_STATE); switch (e.keyCode) { case KeyCodes.LEFT_ARROW: RTLState ? this.next_() : this.previous_(); break; case KeyCodes.RIGHT_ARROW: RTLState ? this.previous_() : this.next_(); break; }

gmajoulet · 2018-06-28T19:01:46Z

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

+      clickEventX: event.pageX,
+    };
+
+    const sections = {


This object could be instantiated only once, from the constructor

newmuis · 2018-06-28T19:46:19Z

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

-    const offsetLeft = ('x' in elRect) ? elRect.x : elRect.left;
-    const offsetWidth = elRect.width;
+    const offsetLeft = ('x' in pageRect) ? pageRect.x : pageRect.left;
+    const RtlState = this.storeService_.get(StateProperty.RTL_STATE);


nit: lowercase 'r'

codecov-io · 2018-06-29T17:48:02Z

Codecov Report

Merging #16381 into master will increase coverage by 0.02%.
The diff coverage is 31.57%.

@@            Coverage Diff             @@
##           master   #16381      +/-   ##
==========================================
+ Coverage   77.14%   77.17%   +0.02%     
==========================================
  Files         547      547              
  Lines       39934    39942       +8     
==========================================
+ Hits        30807    30824      +17     
+ Misses       9127     9118       -9

Impacted Files	Coverage Δ
...xtensions/amp-story/1.0/amp-story-store-service.js	`83.33% <0%> (-1.78%)`	⬇️
extensions/amp-story/1.0/amp-story.js	`61.74% <20%> (-0.56%)`	⬇️
extensions/amp-story/1.0/page-advancement.js	`29.74% <38.46%> (+2.47%)`	⬆️
...p-story/1.0/amp-story-unsupported-browser-layer.js	`71.42% <0%> (-9.53%)`	⬇️
src/base-element.js	`91.17% <0%> (-0.99%)`	⬇️
src/custom-element.js	`94.23% <0%> (-0.2%)`	⬇️
extensions/amp-next-page/0.1/next-page-service.js	`80% <0%> (+2.42%)`	⬆️
src/curve.js	`82.05% <0%> (+2.56%)`	⬆️
extensions/amp-dailymotion/0.1/amp-dailymotion.js	`45.45% <0%> (+2.72%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fefe7f...65ce3e2. Read the comment docs.

Enriqe · 2018-06-29T18:05:34Z

ready to merge @newmuis

Enriqe requested review from gmajoulet and newmuis June 26, 2018 18:05

googlebot added the cla: yes label Jun 26, 2018

Enriqe force-pushed the rtl-nav branch from 4eae51f to 69c3052 Compare June 26, 2018 18:06

newmuis reviewed Jun 26, 2018

View reviewed changes

gmajoulet reviewed Jun 26, 2018

View reviewed changes

Enriqe force-pushed the rtl-nav branch from 15478c9 to d748f14 Compare June 27, 2018 22:15

Enriqe commented Jun 27, 2018

View reviewed changes

gmajoulet reviewed Jun 27, 2018

View reviewed changes

Enriqe force-pushed the rtl-nav branch from 17ce30b to f029814 Compare June 28, 2018 18:36

gmajoulet approved these changes Jun 28, 2018

View reviewed changes

newmuis approved these changes Jun 28, 2018

View reviewed changes

Enriqe added 4 commits June 28, 2018 17:06

supports rtl navigation

bd18659

Uses store service, changes keyboard clicks.

d6850bd

reviews

8f76c40

reviews

1669852

Enriqe force-pushed the rtl-nav branch from f029814 to 1669852 Compare June 28, 2018 21:06

fix tests

65ce3e2

Enriqe force-pushed the rtl-nav branch from a0ca4e2 to 65ce3e2 Compare June 29, 2018 17:16

newmuis merged commit f2ec9fc into ampproject:master Jun 29, 2018

Enriqe deleted the rtl-nav branch June 29, 2018 20:43

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

Adds support for RTL navigation in amp-story pages ✨ #16381

Adds support for RTL navigation in amp-story pages ✨ #16381

Enriqe commented Jun 26, 2018

newmuis left a comment

newmuis Jun 26, 2018

gmajoulet Jun 26, 2018

gmajoulet commented Jun 26, 2018

Enriqe Jun 27, 2018

gmajoulet left a comment

gmajoulet Jun 27, 2018

gmajoulet Jun 27, 2018

Enriqe Jun 28, 2018

gmajoulet Jun 27, 2018

Enriqe Jun 28, 2018

gmajoulet Jun 27, 2018

newmuis Jun 27, 2018

Enriqe Jun 28, 2018

gmajoulet Jun 27, 2018

gmajoulet Jun 27, 2018

gmajoulet Jun 28, 2018

newmuis Jun 28, 2018

codecov-io commented Jun 29, 2018

Enriqe commented Jun 29, 2018

		@@ -580,6 +581,12 @@ export class AmpStory extends AMP.BaseElement {

		this.initializeBookend_();

		const {document} = this.win;

Adds support for RTL navigation in amp-story pages ✨ #16381

Adds support for RTL navigation in amp-story pages ✨ #16381

Conversation

Enriqe commented Jun 26, 2018

newmuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmajoulet commented Jun 26, 2018

Choose a reason for hiding this comment

gmajoulet left a 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

codecov-io commented Jun 29, 2018

Codecov Report

Enriqe commented Jun 29, 2018