-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨[Project Apester] Implement supporting adset data #39814
base: main
Are you sure you want to change the base?
✨[Project Apester] Implement supporting adset data #39814
Conversation
apesterElement.appendChild(ampAvAdWrap); | ||
|
||
showInUnitAd(ampAvAdWrap, progressBarWrap, refreshOptions); | ||
setInterval( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@powerivq do you think this should be in a vsync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would be better. I do not think we do setInterval in the code base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@powerivq @erwinmombay
Hey, guys. I don't understand how in this case launch again function after timeout. Could u help me, also it was before I just moved to one utils file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreyYatskov could you explain why you need interval here? I don't fully understand why showInUnitAd
needs to be repeatedly called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, here we are show an ad on the same place where unit and cover it, so this is making by some interval apprx each 30sec or 1 min
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@powerivq hey, it was before in another file I just moved to one utils https://github.com/ampproject/amphtml/pull/39814/files#diff-37f33fea038b529eea9fcd8ea1b6a593c41299791d08a2069647f127e55bf253. Also there no possibility that player will be destroyed.
So basically showInUnitAd
creating covered background and showing an ad and then hide and should repeat this periodically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreyYatskov AMP runtime does resource recycling and will destroy things in order to reuse the runtime. Therefore, components should clean up the states when they are being destroyed. Here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, @powerivq so what u propose to apply? cause I'm not so familiar with amphtml and don't know the best practice and don't have an idea how to implement periodically appearing/hiding an element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreyYatskov setInterval is okay. You just need to remove the interval by calling clearInterval
in your unlayoutCallback
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree here, as long as we do cleanup i'd be ok with the change
This code is needed for our customers cause new data configuration is implemented.