New amp-story-bling-link element #22871
Conversation
| /** | ||
| * Downloads Material Icons if bling links are present in the story | ||
| */ | ||
| getBlingLinkIcons_() { |
There was a problem hiding this comment.
Maybe it makes sense to do this in amp-story-page's setDistance, so we can only load the fonts when you get close to the page that uses them. Then you could also just check to see if that link tag is already in the DOM, to prevent bloating the DOM with extra elements, or making extra requests (if the browser didn't cache it somehow).
Distance is how many "pages away" the amp-story-page element is from being in view (i.e. the current page has distance 0, the next page has distance 1, the page after that 2, etc.)
There was a problem hiding this comment.
Thanks for the tip! I'll try it out.
There was a problem hiding this comment.
So @gmajoulet gave me the idea of using SVGs in the stylesheet, which is nice because we don't have to do any remote calls and we'll only need a handle of icons anyways. One downside is that we can't use pure CSS to resize or recolor the icon, but updating the SVG directly should be easy enough to do with JS.
For this PR I'd like to keep the icon at its default values (black, 24x24) but adding size/color/icon attributes on amp-story-bling-link is something I'll work on next.
| } | ||
|
|
||
| AMP.extension('amp-story', '1.0', AMP => { | ||
| AMP.registerElement('amp-story', AmpStory, CSS); | ||
| AMP.registerElement('amp-story-access', AmpStoryAccess); | ||
| AMP.registerElement('amp-story-bling-link', AmpStoryBlingLink); |
There was a problem hiding this comment.
OK for now, but I suspect we don't actually want to call it this 😄
There was a problem hiding this comment.
Open to ideas haha.
There was a problem hiding this comment.
Not saying I actually have any 😅
I'll sync with some folks on the monetization side who may be a bit closer to what publishers would actually expect. Definitely not blocking for now
| border-radius: 50% !important; | ||
| text-align: center !important; | ||
| line-height: 75px !important; | ||
| width: 75px !important; |
There was a problem hiding this comment.
Is 75px the real size we're going to use?
There was a problem hiding this comment.
It doesn't have to be, just eyeballing. Waiting for UI specs
|
Quick high level question: have we considered using a |
|
Good question, I opted for a custom element because it needs to create a tooltip element on click, and determine whether to open it to the left or right. I also pictured adding other attributes to the custom element, like preset sizes, icon, dark/light, and color of icon/shadow. |
| @@ -0,0 +1,72 @@ | |||
| /** | |||
There was a problem hiding this comment.
At a high-level, I think we should decide exactly how templated these are supposed to be (perhaps in a separate PR). Is it okay if a publisher slaps a 6px red border around it? If they change the padding? Add a box-shadow?
We might want to think about whether it's worth either:
- Putting this in a separate shadow DOM tree, or
- Think through some specific properties we'd like to enforce (even if we're enforcing their default state)
| } | ||
|
|
||
| AMP.extension('amp-story', '1.0', AMP => { | ||
| AMP.registerElement('amp-story', AmpStory, CSS); | ||
| AMP.registerElement('amp-story-access', AmpStoryAccess); | ||
| AMP.registerElement('amp-story-bling-link', AmpStoryBlingLink); |
There was a problem hiding this comment.
Not saying I actually have any 😅
I'll sync with some folks on the monetization side who may be a bit closer to what publishers would actually expect. Definitely not blocking for now
| } | ||
|
|
||
| amp-story-page[active] amp-story-bling-link { | ||
| animation: pulse 4s infinite !important; |
There was a problem hiding this comment.
Animating on the box-shadow will trigger a layout + paint in every frame during the animation, using too much resources and potentially creating jank in the animation.
You can see what each CSS property trigger during an animation on https://csstriggers.com/
For this kind of animation, I think we want to have a circle that grows/shrinks through a transform: scale(...). As for the timing function, we've been trying to follow the material guidelines, that'd recommend using cubic-bezier(0.4, 0.0, 0.2, 1); :)
Lmk if this makes sense or if I can help!
There was a problem hiding this comment.
This is really cool to know, thanks for the heads up. I think I have a working solution, PTAL! One note is that I'm creating more dom elements so after buildCallback() this is what the dom looks like
<amp-story-bling-link id="blink-1" layout="fixed" width="90px" height="90px">
<div class="i-amphtml-story-bling-link-pulse"></div>
<div class="i-amphtml-story-bling-link-button">
<i class="i-amphtml-story-bling-link-icon i-amphtml-story-bling-link-shopping-cart"></i>
</div>
</amp-story-bling-link>
| */ | ||
| export const BLING_LINK_SELECTOR = 'a.i-amphtml-story-bling-link'; | ||
|
|
||
| export class AmpStoryBlingLink { |
There was a problem hiding this comment.
Converted this from a custom component to an <a> tag to best support users/publishers of this affiliate link (i.e. letting them decide whether to include rel="nofollow" which is only helpful on an <a> tag)
| * Links that are bling links. | ||
| * @const {string} | ||
| */ | ||
| export const BLING_LINK_SELECTOR = 'a.i-amphtml-story-bling-link'; |
There was a problem hiding this comment.
Publishers won't be able to use this for now as this is invalid AMP, we can change it later once the feature is ready :)
* initial custom element * merch1 * add shopping cart icon * smaller merch img * fix types * pr comments * pr comments 2 * working alternative * refactored with mark up * refactored without mark up * pr comments * working implementation on <a> * turn component into link * less querying
Creates a new
amp-story-bling-linkelement with color, size, icon, and animation all hard coded.Next step is to open a tooltip on click.
/related to #22703