Skip to content

Commit

Permalink
Edit amp-sidebar to default to side="right" for stories ✨ (ampproject…
Browse files Browse the repository at this point in the history
…#19595)

* make the default side for the sidebar left for RTL and vice versa

* fixed utils

* visual tests not part of this change

* Oops! Removing console debugging code

* fixed lint issues

* implemented requested PR changes

* edited JSDoc

* fixed lint issues

* implemented PR review changes

* Accidentally changed comments

* fixed lint issues

* fixed lint issues
  • Loading branch information
bramanudom authored and Noran Azmy committed Mar 22, 2019
1 parent 37f8cf2 commit 48b7232
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 4 deletions.
26 changes: 23 additions & 3 deletions extensions/amp-sidebar/0.1/amp-sidebar.js
Expand Up @@ -21,6 +21,7 @@ import {Services} from '../../../src/services';
import {Toolbar} from './toolbar';
import {closestByTag, isRTL, tryFocus} from '../../../src/dom';
import {createCustomEvent} from '../../../src/event-helper';
import {descendsFromStory} from '../../../src/utils/story';
import {dev} from '../../../src/log';
import {dict} from '../../../src/utils/object';
import {removeFragment} from '../../../src/url';
Expand All @@ -33,6 +34,12 @@ const TAG = 'amp-sidebar toolbar';
/** @private @const {number} */
const ANIMATION_TIMEOUT = 350;

/** @private @enum {string} */
const Side = {
LEFT: 'left',
RIGHT: 'right',
};

/**
* For browsers with bottom nav bars the content towards the bottom
* end of the sidebar is cut off.
Expand Down Expand Up @@ -115,8 +122,9 @@ export class AmpSidebar extends AMP.BaseElement {

this.action_ = Services.actionServiceForDoc(element);

if (this.side_ != 'left' && this.side_ != 'right') {
this.side_ = isRTL(this.document_) ? 'right' : 'left';
if (this.side_ != Side.LEFT && this.side_ != Side.RIGHT) {
this.side_ = this.setSideAttribute_(
isRTL(this.document_) ? Side.RIGHT : Side.LEFT);
element.setAttribute('side', this.side_);
}

Expand Down Expand Up @@ -350,7 +358,19 @@ export class AmpSidebar extends AMP.BaseElement {
tryFocus(this.openerElement_);
}
}

/**
* Sidebars within <amp-story> should be 'flipped'.
* @param {!Side} side
* @return {Side}
* @private
*/
setSideAttribute_(side) {
if (!descendsFromStory(this.element)) {
return side;
} else {
return side == Side.LEFT ? Side.RIGHT : Side.LEFT;
}
}
/**
* @private
*/
Expand Down
2 changes: 1 addition & 1 deletion src/utils/story.js
Expand Up @@ -26,7 +26,7 @@ import {closestByTag, waitForChild} from '../dom';
* @return {boolean}
*/
export function descendsFromStory(element) {
return !!closestByTag(element, 'amp-story-page');
return !!closestByTag(element, 'amp-story');
}

/**
Expand Down

0 comments on commit 48b7232

Please sign in to comment.