amp-consent fullscreen interaction e2e#26584
amp-consent fullscreen interaction e2e#26584micajuine-ho merged 18 commits intoampproject:masterfrom
Conversation
zhouyx
left a comment
There was a problem hiding this comment.
Does the PR only add e2e test? Or it also implements the logic? Asking because I thought the real implementation is covered by a different PR.
|
|
||
| parent.postMessage(readyObject, '*'); | ||
| }, 2500); | ||
| }, 500); |
There was a problem hiding this comment.
question: why attempting to set fullscreen every 500ms?
ea05343 to
b9cae21
Compare
|
Is this PR still active? |
|
This pull request introduces 1 alert when merging 4ec66d7 into 69320ba - view on LGTM.com new alerts:
|
140a77f to
e06b0bd
Compare
e06b0bd to
726fdf9
Compare
extensions/amp-consent/0.1/cmps.js
Outdated
| CMP_CONFIG['_test_'] = { | ||
| 'consentInstanceId': '_test_', | ||
| 'checkConsentHref': '/get-consent-v1?cid=CLIENT_ID&pid=PAGE_VIEW_ID', | ||
| 'promptUISrc': '/test/fixtures/served/cmp-iframe.html', |
There was a problem hiding this comment.
could we override the promptUISrc with inline config, and get rid of either _ping_ or _test_
There was a problem hiding this comment.
Ahh yes, good point.
| @@ -0,0 +1,68 @@ | |||
| /** | |||
There was a problem hiding this comment.
I wonder if you want to add additional UI test here. For example the lightbox mode. If so I might rename the file.
But that can also be a visual diff test. So your call
There was a problem hiding this comment.
I like the idea of doing more e2e tests, so I'll change the file name.
ddac21a to
9f344b8
Compare
Last task of #26432
Created an e2e test to test the fullscreen signal and interaction.
Changes:
_test_CMP (only available in test/dev mode) that points at the new.htmlfile for thepromptUiSrc_test_CMP in the e2e test by declaring__AMP_TEST = trueinfullscreen-cmp.htmldiy-consent.htmlfor clarityNotes: