✨ Render close button for lightbox ads in inabox #16886

alanorozco · 2018-07-18T21:46:41Z

No description provided.

codecov-io · 2018-07-19T16:56:19Z

Codecov Report

Merging #16886 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #16886      +/-   ##
==========================================
- Coverage   77.93%   77.92%   -0.02%     
==========================================
  Files         562      562              
  Lines       41156    41140      -16     
==========================================
- Hits        32075    32058      -17     
- Misses       9081     9082       +1

Flag	Coverage Δ
#integration_tests	`36.16% <0%> (-0.01%)`	⬇️
#unit_tests	`76.98% <100%> (-0.01%)`	⬇️

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 11a3de7...c1cac8d. Read the comment docs.

aghassemi · 2018-07-19T21:28:08Z

gulpfile.js

@@ -398,6 +398,29 @@ function css() {
  return compileCss();
 }

+const cssEntryPoints = [


/cc @erwinmombay

lannka · 2018-07-19T21:42:19Z

ads/inabox/frame-overlay-manager.js

@@ -65,6 +65,7 @@ export class FrameOverlayManager {
      this.isExpanded_ = true;
      this.viewportChangedSinceExpand_ = false;
      this.collapsedRect_ = collapsedRect;
+


lannka · 2018-07-19T21:45:30Z

extensions/amp-lightbox/0.1/amp-lightbox.js

+   * @return {boolean}
+   * @private
+   */
+  isInabox_() {


what do you think we make the close button a feature generic to amp-lightbox in both FIE or regular AMP. The feature is enabled by having element attribute "close-button".

We then let the A4A validator to ensure its existence in ads.

This is a great idea, and decreases complexity! Done.

alanorozco

And now the commit line count is negative! Thanks for the idea @lannka.

alanorozco · 2018-07-19T22:14:45Z

ads/inabox/frame-overlay-manager.js

@@ -65,6 +65,7 @@ export class FrameOverlayManager {
      this.isExpanded_ = true;
      this.viewportChangedSinceExpand_ = false;
      this.collapsedRect_ = collapsedRect;
+


alanorozco · 2018-07-19T22:15:12Z

extensions/amp-lightbox/0.1/amp-lightbox.js

+   * @return {boolean}
+   * @private
+   */
+  isInabox_() {


This is a great idea, and decreases complexity! Done.

…inabox-lightbox

lannka · 2018-07-23T17:00:17Z

extensions/amp-lightbox/0.1/amp-lightbox.js

+   * @return {boolean}
+   * @private
+   */
+  isInAd_() {


or, can we just check if the element is in an iframe? Is there a way to check if the owner document is the one AMP runtime is on?

lannka · 2018-07-23T18:41:49Z

extensions/amp-lightbox/0.1/amp-lightbox.js

+   * @private
+   */
+  isInIframe_() {
+    return this.win != this.win.top;


this will not work in Search Viewer.

I'm wondering if there is a way to check which window the AMP runtime is installed
maybe @choumx knows

True, I forgot about that case.

alanorozco · 2018-07-23T21:10:46Z

I have no idea why this set of tests keeps failing on Travis. I haven't found a way to reproduce locally.

These tests are related to Adsense and Doubleclick implementations, very unrelated to this PR. @keithwrightbos Any ideas?

@lannka Anything known about these errors would be valuable info for me.

alanorozco · 2018-07-23T21:28:59Z

@honeybadgerdontcare Added validation rules specific for A4A. Implementation is still guarded by experiment flag.

Validation rules should provide context as to why the tag was forked.

honeybadgerdontcare · 2018-07-24T04:59:08Z

extensions/amp-lightbox/validator-amp-lightbox.protoascii

+tags: { # <amp-lightbox close-button> in A4A.
+  html_format: AMP4ADS
+  html_format: EXPERIMENTAL
+  tag_name: "AMP-LIGHTBOX"


tag_name is unique except when there is also a spec_name. Perhaps spec_name: "amp-lightbox [a4a]" or amp4ads instead of a4a. also, why are both html_format: EXPERIMENTAL?

@Gregable since I'm out tomorrow.

Done.

Not sure why the original tag is marked allowed in experimental.

Gregable · 2018-07-24T19:58:22Z

extensions/amp-lightbox/validator-amp-lightbox.protoascii

+tags: { # <amp-lightbox close-button> in AMP4ADS.
+  html_format: AMP4ADS
+  tag_name: "AMP-LIGHTBOX"
+  spect_name: "amp-lightbox [AMP4ADS]"


Whoops, done.

Gregable · 2018-07-24T19:59:14Z

extensions/amp-lightbox/validator-amp-lightbox.protoascii

@@ -58,3 +58,32 @@ tags: {  # <amp-lightbox>
    supported_layouts: NODISPLAY
  }
 }
+tags: { # <amp-lightbox close-button> in AMP4ADS.


2 spaces before comment

…ghtbox

* Render close button for lightbox ads in inabox * Refactor css entry points * Fix transitions * Decrease complexity by using `close-button` attribute * Put things where they belong * Types, lint * Bundle size * Change transition times * Remove unused * Add validation rules for A4A. * 👨🏻 Add self as owner of <amp-lightbox> * Clarification * Address review * Bundle size * Address review * Mock getComputedStyle

alanorozco requested review from aghassemi and lannka July 18, 2018 21:46

alanorozco force-pushed the inabox-lightbox branch from 7d8a6f8 to 25db4cf Compare July 18, 2018 21:46

googlebot added the cla: yes label Jul 18, 2018

alanorozco force-pushed the inabox-lightbox branch from 25db4cf to 61e7b2c Compare July 18, 2018 21:47

Render close button for lightbox ads in inabox

3594afb

alanorozco force-pushed the inabox-lightbox branch from 61e7b2c to 3594afb Compare July 18, 2018 22:52

ampproject deleted a comment Jul 18, 2018

alanorozco force-pushed the inabox-lightbox branch from 7fa4fee to b6f6a48 Compare July 19, 2018 15:41

Refactor css entry points

9918244

alanorozco force-pushed the inabox-lightbox branch from b6f6a48 to 9918244 Compare July 19, 2018 15:43

alanorozco added 2 commits July 19, 2018 09:02

Line length

8024065

oops

bdfefd8

aghassemi approved these changes Jul 19, 2018

View reviewed changes

gulpfile.js

@@ -398,6 +398,29 @@ function css() {

return compileCss();

}

const cssEntryPoints = [

Copy link

Contributor

aghassemi Jul 19, 2018

Choose a reason for hiding this comment

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

/cc @erwinmombay

lannka reviewed Jul 19, 2018

View reviewed changes

alanorozco added 3 commits July 19, 2018 14:50

Bugfix, style

469688e

Fix transitions

ebf8fdf

Decrease complexity by using close-button attribute

5b57fcf

alanorozco force-pushed the inabox-lightbox branch from 7426f1b to ee6afca Compare July 19, 2018 22:20

alanorozco commented Jul 19, 2018

View reviewed changes

Put things where they belong

b5c9c86

alanorozco force-pushed the inabox-lightbox branch from ee6afca to b5c9c86 Compare July 19, 2018 22:25

alanorozco and others added 7 commits July 19, 2018 16:09

Types, lint

dda67ac

Bundle size

632581d

Merge branch 'master' into inabox-lightbox

4751fc3

Change transition times

46042a7

Merge branch 'master' into inabox-lightbox

548a052

Merge branch 'inabox-lightbox' of github.com:alanorozco/amphtml into …

1bf4b53

…inabox-lightbox

Remove unused

9d8066c

lannka reviewed Jul 23, 2018

View reviewed changes

alanorozco force-pushed the inabox-lightbox branch from 66a4b94 to 9d8066c Compare July 23, 2018 21:03

Add validation rules for A4A.

55f894a

👨🏻 Add self as owner of <amp-lightbox>

d2efb5b

alanorozco force-pushed the inabox-lightbox branch from 22ef2a9 to d2efb5b Compare July 23, 2018 21:32

Clarification

f558081

alanorozco requested a review from honeybadgerdontcare July 23, 2018 22:46

honeybadgerdontcare reviewed Jul 24, 2018

View reviewed changes

lannka approved these changes Jul 24, 2018

View reviewed changes

Address review

a1000c4

alanorozco requested a review from Gregable July 24, 2018 18:31

Skip to see what happens

badafa6

Gregable requested changes Jul 24, 2018

View reviewed changes

alanorozco added 4 commits July 24, 2018 13:13

Bundle size

c339425

Address review

d08ef8e

Mock getComputedStyle

08617b9

Merge branch 'master' of github.com:ampproject/amphtml into inabox-li…

c1cac8d

…ghtbox

Gregable approved these changes Jul 24, 2018

View reviewed changes

alanorozco merged commit d4a3fd5 into ampproject:master Jul 24, 2018

alanorozco deleted the inabox-lightbox branch July 24, 2018 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Render close button for lightbox ads in inabox #16886

✨ Render close button for lightbox ads in inabox #16886

alanorozco commented Jul 18, 2018

codecov-io commented Jul 19, 2018 •

edited

Loading

aghassemi Jul 19, 2018

lannka Jul 19, 2018

alanorozco Jul 19, 2018

lannka Jul 19, 2018

alanorozco Jul 19, 2018

alanorozco left a comment

alanorozco Jul 19, 2018

alanorozco Jul 19, 2018

lannka Jul 23, 2018

alanorozco Jul 23, 2018

alanorozco Jul 23, 2018

lannka Jul 23, 2018

alanorozco Jul 23, 2018

alanorozco commented Jul 23, 2018

alanorozco commented Jul 23, 2018

honeybadgerdontcare Jul 24, 2018

alanorozco Jul 24, 2018

Gregable Jul 24, 2018

alanorozco Jul 24, 2018

Gregable Jul 24, 2018

alanorozco Jul 24, 2018

✨ Render close button for lightbox ads in inabox #16886

✨ Render close button for lightbox ads in inabox #16886

Conversation

alanorozco commented Jul 18, 2018

codecov-io commented Jul 19, 2018 • edited Loading

Codecov Report

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 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

alanorozco commented Jul 23, 2018

alanorozco commented Jul 23, 2018

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 Jul 19, 2018 •

edited

Loading