Skip to content

Url replacement clickxy#4153

Closed
keithwrightbos wants to merge 9 commits intoampproject:masterfrom
google:url-replacement-clickxy
Closed

Url replacement clickxy#4153
keithwrightbos wants to merge 9 commits intoampproject:masterfrom
google:url-replacement-clickxy

Conversation

@keithwrightbos
Copy link
Copy Markdown
Contributor

Implementation of document click listener expanding anchor target href including new X/Y click location. See I2I #3665

Enforces sync/async via type check though that is not currently enabled.

May want to merge within document click listener already created within src/document-click.js... thoughts?

validated via type check system.  Additionally adds document click
listener which will expand href of anchor target.

Added replacement service to experiments.js which ensures it is fully
typed check.  Fixed numerous errors in other files to ensure type check
executed on modified portions.  Managed to get type check failures down
to the following within the file which are mostly unrelated.  At some
point this quarter, AMP will be moving to full type checking.  The
following changes were not included as they were only to facilitate type
checks.

Changes not staged for commit:
       modified:   src/animation.js
       modified:   src/curve.js
       modified:   src/experiments.js
       modified:   src/time.js
       modified:   src/transition.js
       modified:   src/url-replacements.js
       modified:   src/viewer.js
       modified:   src/viewport.js
       modified:   src/vsync.js

src/service/url-replacements-impl.js:68: ERROR - assignment to property getAccessService_ of UrlReplacements$$module$src$service$url_replacements_impl
found   : function (Window): Promise<(AccessService|null)>
required: function ((Window|null)): (Promise<(AccessService|null)>|null)
    this.getAccessService_ = accessServiceForOrNull;
    ^

src/service/url-replacements-impl.js:116: ERROR - inconsistent return type
found   : Promise<string>
required: Promise<(string|undefined)>
      return viewerFor(this.win_).getReferrerUrl();
             ^

src/service/url-replacements-impl.js:188: ERROR - Property get never defined on UserNotificationManager
          return service.get(opt_userNotificationId);
                         ^

src/service/url-replacements-impl.js:192: ERROR - Property get never defined on Cid
        return cid.get({
                   ^

src/service/url-replacements-impl.js:204: ERROR - restricted index type
found   : *
required: string
        user.assert(variants[experiment] !== undefined,
                             ^

src/service/url-replacements-impl.js:207: ERROR - restricted index type
found   : *
required: string
        const variant = variants[experiment];
                                 ^

src/service/url-replacements-impl.js:331: ERROR - inconsistent return type
found   : (Promise<(AccessService|null)>|null)
required: Promise<(string|undefined)>
      return this.getAccessValue_(accessService => {
             ^

src/service/url-replacements-impl.js:332: ERROR - Property getAccessReaderId never defined on AccessService
        return accessService.getAccessReaderId();
                             ^

src/service/url-replacements-impl.js:340: ERROR - inconsistent return type
found   : (Promise<(AccessService|null)>|null)
required: Promise<(string|undefined)>
      return this.getAccessValue_(accessService => {
             ^

src/service/url-replacements-impl.js:341: ERROR - Property getAuthdataField never defined on AccessService
        return accessService.getAuthdataField(field);
                             ^

src/service/url-replacements-impl.js:347: ERROR - inconsistent return type
found   : Promise<string>
required: Promise<(string|undefined)>
      return viewerFor(this.win_).getViewerOrigin();
             ^

src/service/url-replacements-impl.js:353: ERROR - Property getTotalEngagedTime never defined on Activity
        return activity.getTotalEngagedTime();
                        ^

src/service/url-replacements-impl.js:415: ERROR - actual parameter 1 of module$src$event_helper.loadPromise does not match formal parameter
found   : Window
required: Element
      return loadPromise(this.win_).then(() => {
                         ^

src/service/url-replacements-impl.js:491: ERROR - assignment
found   : {async: (function (...*): Promise<(string|undefined)>|undefined), sync: (function (...*): (string|undefined)|null)}
required: {async: function (...*): Promise<(string|undefined)>, sync: function (...*): (string|undefined)}
    this.replacements_[varName] =
    ^

src/service/url-replacements-impl.js:684: ERROR - restricted index type
found   : string
required: number
        if (allKeys[key] === undefined) {
                    ^

src/service/url-replacements-impl.js:807: ERROR - inconsistent return type
found   : *
required: UrlReplacements$$module$src$service$url_replacements_impl
  return getService(window, 'url-replace', () => {
return (typeof params[param] !== 'undefined') ?
params[param] :
defaultValue;
return (typeof val !== 'undefined') ? val : defaultValue;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please invert this so there's not a double negative.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@keithwrightbos
Copy link
Copy Markdown
Contributor Author

Back to @jridgewell for review. Thanks!

if (!this.win_.document) {
dev.warn('no win.document?! testing?');
return;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be in capture phase, so it works when propagation is stopped?

Should it be on document.documentElement?

@jridgewell
Copy link
Copy Markdown
Contributor

Ping.

@keithwrightbos
Copy link
Copy Markdown
Contributor Author

Apologies, have been working on higher priority items. Will have changed
ready for review early next week.

On Jul 27, 2016 5:02 PM, "Justin Ridgewell" notifications@github.com
wrote:

Ping.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4153 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABr0L_J5eXFS0mrtD9WVEnPBFGSrr0IJks5qZ8drgaJpZM4JR8if
.

@jridgewell
Copy link
Copy Markdown
Contributor

Not a problem, just going through my backlog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants