Skip to content
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.

Sign up for GitHub

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

Jump to bottom

✨amp-consent: Added a default loading placeholder for the consent UI #19841

Merged
merged 14 commits into from
Dec 19, 2018
Merged

✨amp-consent: Added a default loading placeholder for the consent UI #19841

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ac28cd3
Added the loading placeholder html
torch2424 Dec 12, 2018
02bd556
Added the svg, and renamed some classes for consistency
torch2424 Dec 12, 2018
3cfdc4f
Finished styles for the placeholder spinner
torch2424 Dec 12, 2018
2285455
Need to stop the height flash/jump thing
torch2424 Dec 12, 2018
6d0ec22
Finished up the loading placeholder
torch2424 Dec 12, 2018
885d44b
Started tests, used htmlFor
torch2424 Dec 12, 2018
92bdfbb
Added tests for placeholder
torch2424 Dec 12, 2018
6f06874
Fixed precommit checks
torch2424 Dec 12, 2018
183219d
Added fix for safari on svg
torch2424 Dec 12, 2018
753ccaf
Another safari fix for svg
torch2424 Dec 12, 2018
accca9a
Made all requested changes
torch2424 Dec 15, 2018
5a35058
Made requested changes
torch2424 Dec 17, 2018
31be5c4
Merge branch 'master' of https://github.com/torch2424/amphtml into am…
torch2424 Dec 17, 2018
f05d29b
Made all requested changes
torch2424 Dec 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 24 additions & 8 deletions extensions/amp-consent/0.1/amp-consent.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,19 +41,35 @@ amp-consent.amp-hidden {
visibility: hidden;
}

amp-consent.i-amphtml-consent-loading {
/* TODO: Finalize the placeholder design */
@keyframes amp-consent-ui-placeholder-spin {
from {
transform: rotate(0deg);
}
to {
transform: rotate(360deg);
}
}

.i-amphtml-consent-placeholder {
/* TODO: Finalize the placeholder design */
.i-amphtml-consent-ui-placeholder {
width: 100%;
height: 100%;

display: flex;
justify-content: center;
align-items: center;
}

.i-amphtml-consent-placeholder:after {
content: "Loading.....";
.i-amphtml-consent-ui-placeholder > svg {
width: 30px;
height: 30px;

fill: none;
stroke-width: 1.5px;
transform-origin: 50% 50%;
animation: 1000ms amp-consent-ui-placeholder-spin linear infinite;
}

.i-amphtml-consent-fill {
.i-amphtml-consent-ui-fill {
position: absolute;
top: 0;
left: 0;
Expand All @@ -76,7 +92,7 @@ amp-consent.i-amphtml-consent-ui-iframe-active.i-amphtml-consent-ui-in.i-amphtml
transform: translate3d(0px, calc(100% - 100vh), 0px) !important;
}

amp-consent.i-amphtml-consent-ui-in.i-amphtml-consent-ui-iframe-fullscreen > .i-amphtml-consent-fill {
amp-consent.i-amphtml-consent-ui-in.i-amphtml-consent-ui-iframe-fullscreen > .i-amphtml-consent-ui-fill {
height: 100vh !important;
}

33 changes: 24 additions & 9 deletions extensions/amp-consent/0.1/consent-ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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 {
Expand Down Expand Up @@ -238,12 +239,24 @@ export class ConsentUI {
* @return {!Element}
*/
createPlaceholder_() {
// TODO(@zhouyx): Allow publishers to provide placeholder upon request
const placeholder = this.parent_.ownerDocument.createElement('placeholder');
Copy link

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.

Copy link
Contributor Author

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?

toggle(placeholder, false);
const {classList} = placeholder;
classList.add(consentUiClasses.fill);
classList.add(consentUiClasses.placeholder);
placeholder.classList.add(consentUiClasses.placeholder);

const loadingSpinner = htmlFor(placeholder)`
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 htmlFor keyword in order to find the template strings to then be optimized. Thus I did it this way to help with that.

Though I may be wrong, we will see what Kris says 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this htmlFor usage is within a method, it will be hoisted automatically in the single pass build. Makes the most sense within source code to place it where it is easiest to understand.

Please note: This optimization is only true in the single-pass build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 ${placeholderHtml}, but the template can't take in variables. I think we may need to do some additional work to allow the codebase to handle this.

Doing a quick project search, seems htmlFor() always has the template immediately after it.

<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;
}

Expand All @@ -261,11 +274,13 @@ 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 this.baseInstance_.mutateElement(() => {
Copy link

Choose a reason for hiding this comment

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

I think this would be a bit more natural as:

return Promise.all([
  this.iframeReady_.promise,
  this.baseInstance_.mutateElement(() => {
    toggle(dev().assertElement(this.placeholder_), true);
  }),
]);

Its not really a performance difference in this case, but seems to more clearly communicate that the two async things are independent.

toggle(dev().assertElement(this.placeholder_), true);
}).then(() => this.iframeReady_.promise);
}

/**
Expand Down
31 changes: 30 additions & 1 deletion extensions/amp-consent/0.1/test/test-consent-ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,10 @@ describes.realWin('consent-ui', {
};
},
scheduleLayout: () => {},
mutateElement: callback => callback(),
mutateElement: callback => {
callback();
return Promise.resolve();
},
};
toggleExperiment(win, 'amp-consent-v2', true);
});
Expand Down Expand Up @@ -142,6 +145,32 @@ describes.realWin('consent-ui', {
});
});

describe('placeholder', () => {
it('should add a placeholder element' +
Copy link

Choose a reason for hiding this comment

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

Having placeholder here feels a bit redundant. The test will read as "consent-ui placeholder should add a placeholder element while loading CMP Iframe". Consider something like "consent-ui placeholder should be present while loading CMP Iframe".

' while loading CMP Iframe', () => {

const config = dict({
'promptUISrc': 'https//promptUISrc',
});
consentUI =
new ConsentUI(mockInstance, config);

const placeholder = consentUI.placeholder_;
expect(placeholder).to.be.ok;
expect(placeholder.hasAttribute('hidden')).to.be.ok;
Copy link

Choose a reason for hiding this comment

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

expect(placeholder.hidden).to.be.true;


consentUI.show();

// Pop onto the back of the event queue,
// so we expect() once our mutate element in show() resolves
return mockInstance.mutateElement(() => {}).then(() => {
Copy link

Choose a reason for hiding this comment

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

Would be better to write this as an async test. Since it is just a test, no need to worry about the code generated to transform async/await into promises. That is:

should('...', async () => {
   ...
  await mockInstance.mutateElement(() => {});
  expect(...);
});

expect(placeholder.hidden).to.be.false;
expect(placeholder.childNodes).to.not.be.empty;
});
});
});


describe('CMP Iframe', () => {

it('should load the iframe, ' +
Expand Down