From 14fbcf11b6c26ce09e3dd6fa7fa346c526974f79 Mon Sep 17 00:00:00 2001 From: Atharva Sune Date: Tue, 19 May 2020 22:44:36 +0530 Subject: [PATCH 1/6] Added css property for amp selector[multiple] to display pointer --- extensions/amp-selector/0.1/amp-selector.css | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/extensions/amp-selector/0.1/amp-selector.css b/extensions/amp-selector/0.1/amp-selector.css index 165438082ece..6465f84cc53a 100644 --- a/extensions/amp-selector/0.1/amp-selector.css +++ b/extensions/amp-selector/0.1/amp-selector.css @@ -23,6 +23,11 @@ amp-selector [option][selected] { outline: solid 1px rgba(0,0,0,0.7); } +amp-selector[multiple] [option][selected] { + cursor: pointer; + outline: solid 1px rgba(0, 0, 0, 0.7); +} + amp-selector [disabled][option], amp-selector[disabled] [option], amp-selector [selected][disabled], From 0aeaacf7e1bc8392f5bdb5c064635ecf96b2b4a5 Mon Sep 17 00:00:00 2001 From: Atharva Sune Date: Thu, 28 May 2020 22:14:00 +0530 Subject: [PATCH 2/6] expose ampTrust property in v2 --- extensions/amp-carousel/0.2/amp-carousel.js | 7 +- .../0.2/test-e2e/test-slidescroll-autoplay.js | 70 +++++++++++++++++++ .../0.2/slidescroll-autoplay.html | 69 ++++++++++++++++++ 3 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 extensions/amp-carousel/0.2/test-e2e/test-slidescroll-autoplay.js create mode 100644 test/fixtures/e2e/amp-carousel/0.2/slidescroll-autoplay.html diff --git a/extensions/amp-carousel/0.2/amp-carousel.js b/extensions/amp-carousel/0.2/amp-carousel.js index e829c34e3270..f2da0f7a9f68 100644 --- a/extensions/amp-carousel/0.2/amp-carousel.js +++ b/extensions/amp-carousel/0.2/amp-carousel.js @@ -547,7 +547,12 @@ class AmpCarousel extends AMP.BaseElement { const action = createCustomEvent(this.win, `slidescroll.${name}`, data); this.action_.trigger(this.element, name, action, trust); - this.element.dispatchCustomEvent(name, data); + + this.element.dispatchCustomEvent(name, { + index: this.currentIndex_, + actionTrust: trust, + }); + this.triggerAnalyticsEvent_(prevIndex, index); } diff --git a/extensions/amp-carousel/0.2/test-e2e/test-slidescroll-autoplay.js b/extensions/amp-carousel/0.2/test-e2e/test-slidescroll-autoplay.js new file mode 100644 index 000000000000..b146cde39a37 --- /dev/null +++ b/extensions/amp-carousel/0.2/test-e2e/test-slidescroll-autoplay.js @@ -0,0 +1,70 @@ +/** + * Copyright 2020 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +describes.endtoend( + 'AMP carousel 0.2 slideChange on type="slide" with autoplay', + { + testUrl: + 'http://localhost:8000/test/fixtures/e2e/amp-carousel/0.2/slidescroll-autoplay.html', + environments: ['single'], + }, + async function (env) { + let controller; + + /** + * Attach an event listener to page to capture the 'slideChange' event. + * If given a selector, click on it to fire the event being listened for. + * @return {!Promise} + */ + function slideChageEventAfterClicking(opt_selector) { + return controller.evaluate((opt_selector) => { + return new Promise((resolve) => { + document.addEventListener( + 'slideChange', + (e) => resolve(e.data), + {once: true} // Remove listener after first invocation + ); + if (opt_selector) { + document.querySelector(opt_selector).click(); + } + }); + }, opt_selector); + } + + beforeEach(async () => { + controller = env.controller; + }); + + it('should fire low trust event for autoplay advance', async () => { + for (let i = 0; i < 3; i++) { + const event = await slideChageEventAfterClicking(/*autoplay*/); + await expect(event.actionTrust).to.equal(1); //ActionTrust.LOW + } + }); + + it('should fire high trust event on user interaction', async () => { + const event = await slideChageEventAfterClicking( + '.amp-carousel-button-next' + ); + await expect(event.actionTrust).to.equal(3); //ActionTrust.HIGH + }); + + it('should fire high trust event on user interaction throw amp-bind', async () => { + const event = await slideChageEventAfterClicking('#go-to-last'); + await expect(event.actionTrust).to.equal(3); //ActionTrust.HIGH + }); + } +); diff --git a/test/fixtures/e2e/amp-carousel/0.2/slidescroll-autoplay.html b/test/fixtures/e2e/amp-carousel/0.2/slidescroll-autoplay.html new file mode 100644 index 000000000000..5d1b22b29354 --- /dev/null +++ b/test/fixtures/e2e/amp-carousel/0.2/slidescroll-autoplay.html @@ -0,0 +1,69 @@ + + + + + Carousel 0.2 + + + + + + + + + +

Autoplay slides

+ +
+ + + + + Sorry, your browser does not support inline SVG. + + + + + + Sorry, your browser does not support inline SVG. + + + + + + Sorry, your browser does not support inline SVG. + + + + + From 3a18ef404a584d6a0c86c7ed27fb09d643a21b42 Mon Sep 17 00:00:00 2001 From: Dhruv Manek Date: Mon, 7 Dec 2020 09:47:29 -0800 Subject: [PATCH 3/6] Expose actionTrust property in amp-carousel v2 `slideChange` event --- extensions/amp-carousel/0.2/amp-carousel.js | 7 +++---- .../0.2/test-e2e/test-slidescroll-autoplay.js | 16 +++++++++------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/extensions/amp-carousel/0.2/amp-carousel.js b/extensions/amp-carousel/0.2/amp-carousel.js index f2da0f7a9f68..4953326dfe48 100644 --- a/extensions/amp-carousel/0.2/amp-carousel.js +++ b/extensions/amp-carousel/0.2/amp-carousel.js @@ -542,12 +542,11 @@ class AmpCarousel extends AMP.BaseElement { const data = dict({'index': index}); const name = 'slideChange'; - const isHighTrust = this.isHighTrustActionSource_(actionSource); - const trust = isHighTrust ? ActionTrust.HIGH : ActionTrust.LOW; - + const trust = this.isHighTrustActionSource_(actionSource) + ? ActionTrust.HIGH + : ActionTrust.LOW; const action = createCustomEvent(this.win, `slidescroll.${name}`, data); this.action_.trigger(this.element, name, action, trust); - this.element.dispatchCustomEvent(name, { index: this.currentIndex_, actionTrust: trust, diff --git a/extensions/amp-carousel/0.2/test-e2e/test-slidescroll-autoplay.js b/extensions/amp-carousel/0.2/test-e2e/test-slidescroll-autoplay.js index b146cde39a37..1258f4e24a88 100644 --- a/extensions/amp-carousel/0.2/test-e2e/test-slidescroll-autoplay.js +++ b/extensions/amp-carousel/0.2/test-e2e/test-slidescroll-autoplay.js @@ -14,6 +14,8 @@ * limitations under the License. */ +import {ActionTrust} from '../../../src/action-constants'; + describes.endtoend( 'AMP carousel 0.2 slideChange on type="slide" with autoplay', { @@ -29,7 +31,7 @@ describes.endtoend( * If given a selector, click on it to fire the event being listened for. * @return {!Promise} */ - function slideChageEventAfterClicking(opt_selector) { + function slideChangeEventAfterClicking(opt_selector) { return controller.evaluate((opt_selector) => { return new Promise((resolve) => { document.addEventListener( @@ -50,21 +52,21 @@ describes.endtoend( it('should fire low trust event for autoplay advance', async () => { for (let i = 0; i < 3; i++) { - const event = await slideChageEventAfterClicking(/*autoplay*/); - await expect(event.actionTrust).to.equal(1); //ActionTrust.LOW + const event = await slideChangeEventAfterClicking(); + await expect(event.actionTrust).to.equal(ActionTrust.LOW); } }); it('should fire high trust event on user interaction', async () => { - const event = await slideChageEventAfterClicking( + const event = await slideChangeEventAfterClicking( '.amp-carousel-button-next' ); - await expect(event.actionTrust).to.equal(3); //ActionTrust.HIGH + await expect(event.actionTrust).to.equal(ActionTrust.HIGH); }); it('should fire high trust event on user interaction throw amp-bind', async () => { - const event = await slideChageEventAfterClicking('#go-to-last'); - await expect(event.actionTrust).to.equal(3); //ActionTrust.HIGH + const event = await slideChangeEventAfterClicking('#go-to-last'); + await expect(event.actionTrust).to.equal(ActionTrust.HIGH); }); } ); From af15680b636d899933095829027f8f7f15097282 Mon Sep 17 00:00:00 2001 From: Dhruv Manek Date: Mon, 7 Dec 2020 14:38:54 -0800 Subject: [PATCH 4/6] Add unit test --- extensions/amp-carousel/0.2/amp-carousel.js | 5 +++-- .../amp-carousel/0.2/test/test-type-slides.js | 13 +++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/extensions/amp-carousel/0.2/amp-carousel.js b/extensions/amp-carousel/0.2/amp-carousel.js index 44d75857cd2f..7d635e20fa24 100644 --- a/extensions/amp-carousel/0.2/amp-carousel.js +++ b/extensions/amp-carousel/0.2/amp-carousel.js @@ -568,11 +568,12 @@ class AmpCarousel extends AMP.BaseElement { const name = 'slideChange'; const isHighTrust = this.isHighTrustActionSource_(actionSource); const trust = isHighTrust ? ActionTrust.HIGH : ActionTrust.LOW; - const data = dict({'index': index, 'actionTrust': trust}); + const data = dict({'index': index}); + const dataWithActionTrust = dict({'index': index, 'actionTrust': trust}); const action = createCustomEvent(this.win, `slidescroll.${name}`, data); this.action_.trigger(this.element, name, action, trust); - dispatchCustomEvent(this.element, name, data); + dispatchCustomEvent(this.element, name, dataWithActionTrust); this.triggerAnalyticsEvent_(prevIndex, index); } diff --git a/extensions/amp-carousel/0.2/test/test-type-slides.js b/extensions/amp-carousel/0.2/test/test-type-slides.js index 5a81f1436942..174d6ad15e89 100644 --- a/extensions/amp-carousel/0.2/test/test-type-slides.js +++ b/extensions/amp-carousel/0.2/test/test-type-slides.js @@ -290,6 +290,19 @@ describes.realWin( expect(eventSpy).to.have.been.calledOnce; }); + + it('should dispatch event with index and actionTrust when changing slides', async () => { + container.addEventListener('slideChange', (event) => { + const {index, actionTrust} = event.data; + expect(index).to.eq(1); + expect(actionTrust).to.eq(ActionTrust.HIGH); + }); + const carousel = await getCarousel({loop: false}); + + carousel.implementation_.interactionNext(); + + await afterIndexUpdate(carousel); + }); }); describe('goToSlide action', () => { From 113aa44cce56cd236a54149eb375714dd4d4ae3b Mon Sep 17 00:00:00 2001 From: Dhruv Manek Date: Mon, 7 Dec 2020 14:46:52 -0800 Subject: [PATCH 5/6] Remove uneeded test files for e2e test --- .../0.2/test-e2e/test-slidescroll-autoplay.js | 72 ------------------- .../0.2/slidescroll-autoplay.html | 69 ------------------ 2 files changed, 141 deletions(-) delete mode 100644 extensions/amp-carousel/0.2/test-e2e/test-slidescroll-autoplay.js delete mode 100644 test/fixtures/e2e/amp-carousel/0.2/slidescroll-autoplay.html diff --git a/extensions/amp-carousel/0.2/test-e2e/test-slidescroll-autoplay.js b/extensions/amp-carousel/0.2/test-e2e/test-slidescroll-autoplay.js deleted file mode 100644 index 9e866a88fdb4..000000000000 --- a/extensions/amp-carousel/0.2/test-e2e/test-slidescroll-autoplay.js +++ /dev/null @@ -1,72 +0,0 @@ -/** - * Copyright 2020 The AMP HTML Authors. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS-IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import {ActionTrust} from '../../../../src/action-constants'; - -describes.endtoend( - 'AMP carousel 0.2 slideChange on type="slide" with autoplay', - { - testUrl: - 'http://localhost:8000/test/fixtures/e2e/amp-carousel/0.2/slidescroll-autoplay.html', - environments: ['single'], - }, - async function (env) { - let controller; - - /** - * Attach an event listener to page to capture the 'slideChange' event. - * If given a selector, click on it to fire the event being listened for. - * @return {!Promise} - */ - function slideChangeEventAfterClicking(opt_selector) { - return controller.evaluate((opt_selector) => { - return new Promise((resolve) => { - document.addEventListener( - 'slideChange', - (e) => resolve(e.data), - {once: true} // Remove listener after first invocation - ); - if (opt_selector) { - document.querySelector(opt_selector).click(); - } - }); - }, opt_selector); - } - - beforeEach(async () => { - controller = env.controller; - }); - - it('should fire low trust event for autoplay advance', async () => { - for (let i = 0; i < 3; i++) { - const event = await slideChangeEventAfterClicking(); - await expect(event.actionTrust).to.equal(ActionTrust.LOW); - } - }); - - it('should fire high trust event on user interaction', async () => { - const event = await slideChangeEventAfterClicking( - '.amp-carousel-button-next' - ); - await expect(event.actionTrust).to.equal(ActionTrust.HIGH); - }); - - it('should fire high trust event on user interaction throw amp-bind', async () => { - const event = await slideChangeEventAfterClicking('#go-to-last'); - await expect(event.actionTrust).to.equal(ActionTrust.HIGH); - }); - } -); diff --git a/test/fixtures/e2e/amp-carousel/0.2/slidescroll-autoplay.html b/test/fixtures/e2e/amp-carousel/0.2/slidescroll-autoplay.html deleted file mode 100644 index 5d1b22b29354..000000000000 --- a/test/fixtures/e2e/amp-carousel/0.2/slidescroll-autoplay.html +++ /dev/null @@ -1,69 +0,0 @@ - - - - - Carousel 0.2 - - - - - - - - - -

Autoplay slides

- -
- - - - - Sorry, your browser does not support inline SVG. - - - - - - Sorry, your browser does not support inline SVG. - - - - - - Sorry, your browser does not support inline SVG. - - - - - From 25aa1c42274a6fa022539a1a60dfcd2980dc3865 Mon Sep 17 00:00:00 2001 From: Dhruv Manek Date: Tue, 8 Dec 2020 10:46:59 -0800 Subject: [PATCH 6/6] Address code review comments --- .../amp-carousel/0.2/test/test-type-slides.js | 24 +++++++------------ 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/extensions/amp-carousel/0.2/test/test-type-slides.js b/extensions/amp-carousel/0.2/test/test-type-slides.js index 174d6ad15e89..2354b9a222c5 100644 --- a/extensions/amp-carousel/0.2/test/test-type-slides.js +++ b/extensions/amp-carousel/0.2/test/test-type-slides.js @@ -280,28 +280,22 @@ describes.realWin( expect(eventSpy).to.have.not.been.called; }); - it('should dispatch when changing slides', async () => { - const eventSpy = env.sandbox.spy(); - container.addEventListener('slideChange', eventSpy); - const carousel = await getCarousel({loop: false}); - - carousel.implementation_.interactionNext(); - await afterIndexUpdate(carousel); - - expect(eventSpy).to.have.been.calledOnce; - }); - it('should dispatch event with index and actionTrust when changing slides', async () => { - container.addEventListener('slideChange', (event) => { - const {index, actionTrust} = event.data; - expect(index).to.eq(1); - expect(actionTrust).to.eq(ActionTrust.HIGH); + let event; + let counter = 0; + container.addEventListener('slideChange', (e) => { + counter++; + event = e; }); const carousel = await getCarousel({loop: false}); carousel.implementation_.interactionNext(); await afterIndexUpdate(carousel); + + expect(counter).to.equal(1); + expect(event.data.index).to.equal(1); + expect(event.data.actionTrust).to.equal(ActionTrust.HIGH); }); });