-
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
✨amp-consent: Added a default loading placeholder for the consent UI #19841
Changes from all commits
ac28cd3
02bd556
3cfdc4f
2285455
6d0ec22
885d44b
92bdfbb
6f06874
183219d
753ccaf
accca9a
5a35058
31be5c4
f05d29b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ import { | |
removeElement, | ||
} from '../../../src/dom'; | ||
import {getData} from '../../../src/event-helper'; | ||
import {htmlFor} from '../../../src/static-template'; | ||
import {isExperimentOn} from '../../../src/experiments'; | ||
import {setStyles, toggle} from '../../../src/style'; | ||
|
||
|
@@ -36,9 +37,9 @@ export const consentUiClasses = { | |
iframeFullscreen: 'i-amphtml-consent-ui-iframe-fullscreen', | ||
iframeActive: 'i-amphtml-consent-ui-iframe-active', | ||
in: 'i-amphtml-consent-ui-in', | ||
loading: 'i-amphtml-consent-loading', | ||
fill: 'i-amphtml-consent-fill', | ||
placeholder: 'i-amphtml-consent-placeholder', | ||
loading: 'i-amphtml-consent-ui-loading', | ||
fill: 'i-amphtml-consent-ui-fill', | ||
placeholder: 'i-amphtml-consent-ui-placeholder', | ||
}; | ||
|
||
export class ConsentUI { | ||
|
@@ -237,12 +238,24 @@ export class ConsentUI { | |
* @return {!Element} | ||
*/ | ||
createPlaceholder_() { | ||
// TODO(@zhouyx): Allow publishers to provide placeholder upon request | ||
const placeholder = this.parent_.ownerDocument.createElement('placeholder'); | ||
toggle(placeholder, false); | ||
const {classList} = placeholder; | ||
classList.add(consentUiClasses.fill); | ||
classList.add(consentUiClasses.placeholder); | ||
placeholder.classList.add(consentUiClasses.placeholder); | ||
|
||
const loadingSpinner = htmlFor(placeholder)` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Is it possible to declare the string as a const at the top of the file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe @kristoferbaxter was looking into doing a compilation pass to do this, so it may not be worth worrying about. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I originally had it as a const at the top of the file, but from what @kristoferbaxter showed me, it appeared to be using the Though I may be wrong, we will see what Kris says 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this Please note: This optimization is only true in the single-pass build. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome! Thanks! So I tried hoisting this to the top of the file, and the linter then throws syntax errors. Also, tried binding it with Doing a quick project search, seems |
||
<svg viewBox="0 0 40 40"> | ||
<defs> | ||
<linearGradient id="grad"> | ||
<stop stop-color="rgb(105, 105, 105)"></stop> | ||
<stop offset="100%" | ||
stop-color="rgb(105, 105, 105)" | ||
stop-opacity="0"></stop> | ||
</linearGradient> | ||
</defs> | ||
<path d="M11,4.4 A18,18, 0,1,0, 38,20" stroke="url(#grad)"></path> | ||
</svg>`; | ||
|
||
placeholder.appendChild(loadingSpinner); | ||
return placeholder; | ||
} | ||
|
||
|
@@ -260,11 +273,16 @@ export class ConsentUI { | |
dev().assertElement(this.placeholder_), null); | ||
} | ||
classList.add(consentUiClasses.loading); | ||
toggle(dev().assertElement(this.placeholder_), true); | ||
toggle(dev().assertElement(this.ui_), false); | ||
this.win_.addEventListener('message', this.boundHandleIframeMessages_); | ||
insertAfterOrAtStart(this.parent_, dev().assertElement(this.ui_), null); | ||
return this.iframeReady_.promise; | ||
|
||
return Promise.all([ | ||
this.iframeReady_.promise, | ||
this.baseInstance_.mutateElement(() => { | ||
toggle(dev().assertElement(this.placeholder_), true); | ||
}), | ||
]); | ||
} | ||
|
||
/** | ||
|
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.
I'm not sure if it is too late to change this, but you shouldn't use a tag name like
placeholder
. A browser could add one with certain semantics that might be a problem. You should only ever use valid html tag names or include a dash in the name.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.
I agree we should change this.
However, @zhouyx do you see any problem in changing this? Is there more code already in place around this idea?