Overwrite or set attr target=_blank in outlinks for amp-story #14382
Conversation
|
/cc @newmuis @gmajoulet |
| setOrOverwriteTargetAttribute_(){ | ||
| const ctaLinks = this.element.querySelectorAll('a'); | ||
| for (let i = 0; i < ctaLinks.length; i++) { | ||
| ctaLinks[i].target = '_blank'; |
There was a problem hiding this comment.
@calebcordry I think you'll need to make sure that you do this for amp-story-auto-ads separately, right? It's probable that the amp-story-cta-layer buildCallback will complete before the amp-story-auto-ads actually injects all of the ads it will inject for a given story.
There was a problem hiding this comment.
I am creating a cta-layer in amp-story-auto-ads so I think at that point the buildCallback will be fired again?
| setOrOverwriteTargetAttribute_(){ | ||
| const ctaLinks = this.element.querySelectorAll('a'); | ||
| for (let i = 0; i < ctaLinks.length; i++) { | ||
| ctaLinks[i].target = '_blank'; |
There was a problem hiding this comment.
Today I learned: buildCallback is running in a Vsync mutate context, otherwise this code would have required to be running in a vsync task
| setOrOverwriteTargetAttribute_(){ | ||
| const ctaLinks = this.element.querySelectorAll('a'); | ||
| for (let i = 0; i < ctaLinks.length; i++) { | ||
| ctaLinks[i].target = '_blank'; |
There was a problem hiding this comment.
Nit: I have no idea if it makes a difference, but the rest of the codebase uses element.setAttribute('target', '_blank'). Maybe we should write it this way too?
| @@ -0,0 +1,58 @@ | |||
| /** | |||
There was a problem hiding this comment.
could you guys take a look at the test too please @gmajoulet @newmuis
| }, | ||
| }, env => { | ||
| let win; | ||
| let ampStoryCtaLayer; |
There was a problem hiding this comment.
Nit: could be a const in the beforeEach, since you're then using ampStoryCtaLayer.element?
| ampStoryCtaLayer.buildCallback(); | ||
| return ampStoryCtaLayer.layoutCallback().then(() => { | ||
| expect( | ||
| ampStoryCtaLayer.element.classList.contains('i-amphtml-story-layer')) |
There was a problem hiding this comment.
Nit: here and below expect(ampStoryCtaLayer.element).to.have.class('i-amphtml-story-layer')
| }); | ||
| }); | ||
|
|
||
| }); |
There was a problem hiding this comment.
Nit: please add a new line at the end of the file :) There is probably a setting for your IDE
There was a problem hiding this comment.
Oops sorry, I always forget. Yes I will search if I can set this on my editor.
Enriqe
force-pushed
the
cta-layer-target-overwrite
branch
from
18b97b6
to
66ee725
Compare
* overwrite or set attr target=_blank in outlinks for amp-story. * console loggg * change method name * jsdoc * Adds test for cta layer. * more verbose in tests * linter * Adds test case.
|
Developers lose control over the flow because of this overriding this. Should consider reverting this change and let the developer decide when target should be _blank vs _self. |
|
@newmuis can you explain the rational behind this design choice? I'm hearing from some developers that is frustrating not having a choice here. Will target=_top be available in the foreseeable future, as discussed in this other issue? |
Fixes #13775
Also imports
amp-story-cta-layerinamp-story.