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

torch2424 · 2018-12-12T20:19:04Z

relates to #18828

This adds a loading spinner provided by @sparhami , and available as a jsbin: https://jsbin.com/yerevoz/1/edit?html,output

Simply adds this svg and styling whenever a loading placeholder is not provided. Note: This placeholder is 30px x 30px as that is the only default height we can guarantee for the amp consent footer thing. Any ideas on how to size the placeholder would be appreciated 😄 👍

sparhami · 2018-12-12T21:02:17Z

extensions/amp-consent/0.1/consent-ui.js

+      <defs>
+        <linearGradient id="grad">
+          <stop stop-color="rgb(105, 105, 105)"></stop>
+          <stop offset="100%"  stop-color="rgba(105, 105, 105, 0)"></stop>


I had to update the code to make it work on Safari correctly. You'll need to do:

stop-color="rgb(105, 105, 105) stop-opacity="0"

instead here

Thanks! Will update now 😄

zhouyx

Looks good to me with a few nits. Before merging the change, please make sure to sync with @spacedino and show them the demo on several real devices.

zhouyx · 2018-12-12T22:39:19Z

extensions/amp-consent/0.1/consent-ui.js

-  placeholder: 'i-amphtml-consent-placeholder',
+  loading: 'i-amphtml-consent-ui-loading',
+  fill: 'i-amphtml-consent-ui-fill',
+  loadingPlaceholder: 'i-amphtml-consent-ui-loading-placeholder',


Nit: Well, placeholder by default means loading placeholder to me. Can we name it placeholder: i-amphtml-consent-ui-placeholder instead : ) A shorter name

zhouyx · 2018-12-12T22:40:43Z

extensions/amp-consent/0.1/consent-ui.js

-    classList.add(consentUiClasses.placeholder);
+    classList.add(consentUiClasses.loadingPlaceholder);
+
+    const loadingSpinner = htmlFor(placeholder)`


nit: Is it possible to declare the string as a const at the top of the file?

I believe @kristoferbaxter was looking into doing a compilation pass to do this, so it may not be worth worrying about.

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 😄

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.

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.

sparhami · 2018-12-14T22:28:55Z

extensions/amp-consent/0.1/amp-consent.css

@@ -41,19 +41,35 @@ amp-consent.amp-hidden {
  visibility: hidden;
 }

-amp-consent.i-amphtml-consent-loading {
-  /* TODO: Finalize the placeholder design */
+@keyframes loading-placeholder-spin  {


maybe prefix with amp-consent into the name of this, since keyframe names are global

sparhami · 2018-12-14T22:30:15Z

extensions/amp-consent/0.1/test/test-consent-ui.js

+      // Pop onto the back of the event queue,
+      // so we expect() once our mutate element in show() resolves
+      return mockInstance.mutateElement(() => {}).then(() => {
+        expect(placeholder.hasAttribute('hidden')).to.not.be.ok;


how about expect(placeholder.hidden).to.be.false?

sparhami · 2018-12-14T22:31:02Z

extensions/amp-consent/0.1/test/test-consent-ui.js

+      // so we expect() once our mutate element in show() resolves
+      return mockInstance.mutateElement(() => {}).then(() => {
+        expect(placeholder.hasAttribute('hidden')).to.not.be.ok;
+        expect(placeholder.childNodes.length).to.be.above(0);


try expect(placeholder.childNodes).to.not.be.empty

kristoferbaxter · 2018-12-14T22:44:52Z

extensions/amp-consent/0.1/consent-ui.js

@@ -238,12 +239,25 @@ 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;


And can remove this now.

Suggested change

const {classList} = placeholder;

kristoferbaxter · 2018-12-14T22:44:55Z

extensions/amp-consent/0.1/consent-ui.js

    const placeholder = this.parent_.ownerDocument.createElement('placeholder');
    toggle(placeholder, false);
    const {classList} = placeholder;
-    classList.add(consentUiClasses.fill);
-    classList.add(consentUiClasses.placeholder);
+    classList.add(consentUiClasses.loadingPlaceholder);


Since you're only adding a single class to the classList, you can remove the line above and just call directly.

Suggested change

classList.add(consentUiClasses.loadingPlaceholder);

placeholder.classList.add(consentUiClasses.loadingPlaceholder);

kristoferbaxter · 2018-12-14T22:45:50Z

extensions/amp-consent/0.1/consent-ui.js

+        </linearGradient>
+      </defs>
+      <path d="M11,4.4 A18,18, 0,1,0, 38,20" stroke="url(#grad)"></path>
+    </svg>`;


If we're using this definition in multiple places, it also might be a good idea to store it externally and import into this file.

As of now, we are only thinking of using this here. Was asking @sparhami about this in the past, if we had a system in place for re-used html elements, and it appears that we do not. So we should probably do that as well if we continue to add more HTML literals.

kristoferbaxter

Left some details about htmlFor and a change for the classList usage.

torch2424 · 2018-12-15T01:27:03Z

@sparhami @zhouyx @kristoferbaxter

Replied to comments / Made requested changes 😄

sparhami · 2018-12-17T04:21:53Z

extensions/amp-consent/0.1/consent-ui.js

    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(() => {


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.

sparhami · 2018-12-17T04:24:39Z

extensions/amp-consent/0.1/consent-ui.js

@@ -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');


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.

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?

sparhami · 2018-12-17T04:27:23Z

extensions/amp-consent/0.1/test/test-consent-ui.js

@@ -142,6 +145,32 @@ describes.realWin('consent-ui', {
    });
  });

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


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

sparhami · 2018-12-17T04:27:55Z

extensions/amp-consent/0.1/test/test-consent-ui.js

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


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

sparhami · 2018-12-17T04:30:59Z

extensions/amp-consent/0.1/test/test-consent-ui.js

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


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(...); });

…p-consent-loading-placeholder

torch2424 · 2018-12-17T23:38:52Z

@sparhami @zhouyx @kristoferbaxter

Replied to comments / Made requested changes 😄

torch2424 · 2018-12-18T01:14:17Z

Thanks for the approval @sparhami !

Going to wait for a design approval from @spacedino and then pull this in (for anyone wondering)

torch2424 · 2018-12-19T01:08:07Z

Had the design review, Loading Placeholder and transition were approved. With some last design changes to be implemented documented here: #19969

Merging...

…mpproject#19841) * Added the loading placeholder html * Added the svg, and renamed some classes for consistency * Finished styles for the placeholder spinner * Need to stop the height flash/jump thing * Finished up the loading placeholder * Started tests, used htmlFor * Added tests for placeholder * Fixed precommit checks * Added fix for safari on svg * Another safari fix for svg * Made all requested changes * Made requested changes * Made all requested changes

torch2424 added 8 commits December 11, 2018 18:11

Added the loading placeholder html

ac28cd3

Added the svg, and renamed some classes for consistency

02bd556

Finished styles for the placeholder spinner

3cfdc4f

Need to stop the height flash/jump thing

2285455

Finished up the loading placeholder

6d0ec22

Started tests, used htmlFor

885d44b

Added tests for placeholder

92bdfbb

Fixed precommit checks

6f06874

torch2424 assigned zhouyx Dec 12, 2018

torch2424 requested a review from zhouyx December 12, 2018 20:19

googlebot added the cla: yes label Dec 12, 2018

sparhami reviewed Dec 12, 2018

View reviewed changes

torch2424 added 2 commits December 12, 2018 13:21

Added fix for safari on svg

183219d

Another safari fix for svg

753ccaf

zhouyx reviewed Dec 12, 2018

View reviewed changes

sparhami reviewed Dec 14, 2018

View reviewed changes

kristoferbaxter reviewed Dec 14, 2018

View reviewed changes

Made all requested changes

accca9a

sparhami suggested changes Dec 17, 2018

View reviewed changes

torch2424 added 3 commits December 17, 2018 15:34

Made requested changes

5a35058

Merge branch 'master' of https://github.com/torch2424/amphtml into am…

31be5c4

…p-consent-loading-placeholder

Made all requested changes

f05d29b

sparhami approved these changes Dec 17, 2018

View reviewed changes

torch2424 mentioned this pull request Dec 19, 2018

AMP-CONSENT v2 UI Followup Design Changes #19969

Closed

5 tasks

torch2424 merged commit a8d1fd3 into ampproject:master Dec 19, 2018

torch2424 deleted the amp-consent-loading-placeholder branch December 19, 2018 01:08

torch2424 mentioned this pull request Dec 19, 2018

AMP-CONSENT v2 UI support #18828

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

torch2424 commented Dec 12, 2018

sparhami Dec 12, 2018

torch2424 Dec 12, 2018

zhouyx left a comment

zhouyx Dec 12, 2018

zhouyx Dec 12, 2018

sparhami Dec 12, 2018

torch2424 Dec 12, 2018

kristoferbaxter Dec 14, 2018

torch2424 Dec 15, 2018

sparhami Dec 14, 2018

sparhami Dec 14, 2018

sparhami Dec 14, 2018

kristoferbaxter Dec 14, 2018

kristoferbaxter Dec 14, 2018

kristoferbaxter Dec 14, 2018

torch2424 Dec 15, 2018

kristoferbaxter left a comment

torch2424 commented Dec 15, 2018

sparhami Dec 17, 2018

sparhami Dec 17, 2018

torch2424 Dec 17, 2018

sparhami Dec 17, 2018

sparhami Dec 17, 2018

sparhami Dec 17, 2018

torch2424 commented Dec 17, 2018

torch2424 commented Dec 18, 2018

torch2424 commented Dec 19, 2018

	classList.add(consentUiClasses.loadingPlaceholder);
	placeholder.classList.add(consentUiClasses.loadingPlaceholder);

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

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

Conversation

torch2424 commented Dec 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoferbaxter left a comment

Choose a reason for hiding this comment

torch2424 commented Dec 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torch2424 commented Dec 17, 2018

torch2424 commented Dec 18, 2018

torch2424 commented Dec 19, 2018