[amp-sidebar 1.0][Toolbar] Added Body Spacing for Default Toolbar #10100

torch2424 · 2017-06-22T22:17:18Z

Creates placeholder elements to push the body down
Placeholder elements are recalculated on layout resize
Made a better (AMP Start) Test manual file for toolbar, satisfying all manual test requirements.

Please see torch2424#8
for examples

camelburrito · 2017-06-22T22:46:20Z

Gave @torch2424 in-person feedback on this

torch2424 · 2017-06-23T20:51:27Z

Please note in my latest changes, I created a new attribute called toolbar-container for when we are creating the container/target for the toolbar. This will allow our users to target the toolbar independently with CSS. I was unable to create the new preety view for toolbar without this. And it would be required for #10126 , in order to apply styles (such as position:fixed) to only our generated containers.

camelburrito · 2017-06-23T21:35:27Z

extensions/amp-sidebar/1.0/amp-sidebar.css

 }

- .i-amphtml-toolbar > ul {
+ .i-amphtml-toolbar > ul:first-child {


shouldnt toolbar only have one nav element inside of it?

i-amphtml-toolbar is the class applied to the <nav toolbar> element. I noticed when implementing the sidebarOnly amp start, that these styles we being applied to the ul within the amp-accordian. So I ensured that this was the first child, as <nav toolbar> may only contain one direct child of <ul>

The way you should ensure it is .i-amphtml-toolbar > :not(ul:first-child) right (or somecss that works?)

That would target the elements that are not the first child of our <nav toolbar> .

Oh wait I was incorrect: https://developer.mozilla.org/en-US/docs/Web/CSS/Child_selectors

It may have been a mistmatch in classes from the AMP start heme, I'll play with it and see what happens

camelburrito · 2017-06-23T21:35:48Z

extensions/amp-sidebar/1.0/amp-sidebar.css

@@ -53,10 +53,10 @@
   z-index: 2147483646;
   width: 100%;
   position: fixed;
-   top: 0;
+   background-color: white;


Yeah we can do that!

camelburrito · 2017-06-23T21:36:02Z

extensions/amp-sidebar/1.0/amp-sidebar.css

@@ -53,10 +53,10 @@
   z-index: 2147483646;
   width: 100%;
   position: fixed;
-   top: 0;


Why is this deleted?

top: 0 breaks the place holder, and doesn't keep it within it's container. Therefore the multiple toolbars will over lap if they all have the same top value.

That is fine , it is up to the developer to adjust this , but by default it should be 0.

camelburrito · 2017-06-23T21:36:13Z

extensions/amp-sidebar/1.0/amp-sidebar.js

@@ -110,6 +110,7 @@ export class AmpSidebar extends AMP.BaseElement {
    const toolbarElements =
    Array.prototype.slice
      .call(this.element.querySelectorAll('nav[toolbar]'), 0);
+    toolbarElements.reverse();


you should not do any DOM re-ordering

I am not re-ording the dom. I am reversing the elements within the array itself. I am doing this because then the toolbars would be added to the DOM in reverse order.

I can either do this, or traverse backward with a for(var i = toolbarElements.length) ... loop

Why would it be added in the reverse order?

Because we are adding each element as the first child of the body.

So if we have toolbars in our html as:

<body>
first
second
third
...
<body>

The loop goes as follows when sidebar is applied:

Iteration 1

<body>
first
...
<body>

Iteration 2

<body>
second
first
...
<body>

Iteration 3

<body>
third
second
first
...
<body>

Reversing the array does this in the correct order

you should not add it as the first child , you will need to add it as the last child so that you dont break existing css. The order of insertion of toolbars should not matter

If we do not add the element as the first child than the placeholder solution will not work.

The alternative in that case would to be to add border-top to the body. In that case we would have to maintain the top: styling of all our toolbars, as well as getting all of our toolbars to play nicely with one another, or having sidebar manage each toolbar's height to calculate top accordingly. Also, in this case, performing #9983 would be much more difficult.

Could you share on what type of existing css would be broken by this method? The only case I could think of is body:first-child

camelburrito · 2017-06-23T21:37:19Z

extensions/amp-sidebar/1.0/toolbar.js

@@ -47,6 +50,9 @@ export class Toolbar {
    /** @private {Element|undefined} */
    this.targetElement_ = undefined;

+    /** @private {Element|undefined} */
+    this.placeholder_ = undefined;


what is a placeholder here ? there is already a set definition for a placeholder in amp.

Let's talk offline about this as I cannot find it.

and this placehodler is used for the elements that will reserve height for our toolbar.

https://www.ampproject.org/docs/guides/responsive/placeholders

Oh that is super cool. Thanks!

I added the placeholder, but it does not remove the need for keeping a reference to the placeholder. Since we are resizing it's height as needed.

However, with accordance to your other comment about adding and removing the placeholder dynamically on attemptShow(), this would make sense to move since we would be creating it there, and not in the toolbar build callback. I'll remove this if we decide to go that route instead of hiding the parent container we create.

ok lets discuss on Monday, also don't call it placeholder :)

Sounds good 👍

I'll rename as I totally forgot to add the placeholder attribute anyways

camelburrito · 2017-06-23T21:41:25Z

extensions/amp-sidebar/1.0/toolbar.js

+
+    // Use the placeholder to fill the height of the toolbar
+    this.vsync_.mutate(() => {
+      if (this.placeholder_) {


This should be done in attempt show , you dont want to display the compensatingElement when toolbar is hidden.

Check if there is a compensation element , if not create one and assign the height

else just assign the height

When you close the toolbar this has to be hidden

The compensating element is within our container that receives display: none; So it would not be shown.

But I can throw this into attempt show before the return.

camelburrito · 2017-06-23T21:42:33Z

Lets not combine features and manual test files in the same PR. Please split it

torch2424 · 2017-06-24T00:09:59Z

Split to: torch2424#8

torch2424 · 2017-06-27T00:15:36Z

Decided the placehodlers would disrupt the body:first-child CSS, and we would support only one default toolbar, where it would set the top attribute of the body to the height of the toolbar. Multiple toolbars will conflict with one another, and fight for the space.

torch2424 · 2017-06-27T00:51:28Z

The build currently isn't passing, but it appears to be an issue with autosuggest, and not the sidebar component. This is ready for review/merge if able.

camelburrito · 2017-06-27T02:29:42Z

extensions/amp-sidebar/1.0/toolbar.js

+
+    // Calculate our body top before returning the fragment
+    this.bodyTop_ =
+      this.win_.getComputedStyle(this.body_, null).getPropertyValue('top');


use the dom.js function

camelburrito · 2017-06-27T02:30:30Z

extensions/amp-sidebar/1.0/toolbar.js

+    // Make room for the toolbar
+    this.vsync_.mutate(() => {
+      if (this.body_) {
+        const toolbarHeight = this.toolbarClone_./*REVIEW*/offsetHeight;


This should be in measure?

camelburrito · 2017-06-27T02:32:00Z

extensions/amp-sidebar/1.0/toolbar.js

@@ -157,6 +183,15 @@ export class Toolbar {
          toggle(element, true);
        });
      }
+
+      // Remove room for our toolbar
+      if (this.body_) {


you should not have to check if body exists.

The check-types linter wont pass without this

camelburrito · 2017-06-27T17:26:41Z

extensions/amp-sidebar/1.0/toolbar.js

@@ -32,6 +35,12 @@ export class Toolbar {
    /** @private {!Object} **/
    this.win_ = win;

+    /** @private {Element|null} */


drop the null here and the linter would pass

If you drop the null here you get:

found : (Element|null)
required: Element
setStyles(this.body_, {

Error in the linter: ERROR - actual parameter 1 of module$src$style.setStyles does not match formal parameter

camelburrito · 2017-06-27T20:15:41Z

extensions/amp-sidebar/1.0/toolbar.js

+          this.initialBodyTop_ = computedStyle(this.win_, this.body_)['top'];
+        }
+        state.toolbarHeight = this.toolbarClone_./*REVIEW*/offsetHeight;
+      },


torch2424 assigned camelburrito Jun 22, 2017

torch2424 requested a review from camelburrito June 22, 2017 22:17

googlebot added the cla: yes label Jun 22, 2017

torch2424 added this to PRs in AMP Sidebar 1.0 Jun 23, 2017

camelburrito reviewed Jun 23, 2017

View reviewed changes

torch2424 changed the title ~~[amp-sidebar 1.0][Toolbar] Added Body Placeholders, Styling, and an AMP Start Manual Test~~ [amp-sidebar 1.0][Toolbar] Added Body Placeholders, Styling Jun 23, 2017

torch2424 changed the title ~~[amp-sidebar 1.0][Toolbar] Added Body Placeholders, Styling~~ [amp-sidebar 1.0][Toolbar] Added Body Placeholders Jun 23, 2017

torch2424 mentioned this pull request Jun 23, 2017

Re-added our manual tests from sidebar-v1-placeholder torch2424/amphtml#8

Closed

torch2424 changed the title ~~[amp-sidebar 1.0][Toolbar] Added Body Placeholders~~ [amp-sidebar 1.0][Toolbar] Added Body Spacing for Default Toolbar Jun 27, 2017

camelburrito suggested changes Jun 27, 2017

View reviewed changes

camelburrito reviewed Jun 27, 2017

View reviewed changes

torch2424 added 8 commits June 27, 2017 11:06

Got a height calculating placehodler solution working

d868cb5

Got set styles working on the header including on scroll

110f907

Fixed some small things, and created a nice ampstart example

e4279be

Fixed amp precommit errors

19bf0cc

Fixed linting errors not on local

2482623

Added attribute to toolbar-targets, and finioshed desktop view

719ac8b

Finished the pretty ampstart sidebar 1.0 example

4b613ad

Fixed a small z-index bug with the accordians

c51d74c

torch2424 added 8 commits June 27, 2017 11:07

Reverted manual test files, and placed in its own branch

6b7b236

Made @camelburrito 's changes

0a65536

Renamed placholder, and added the placeholder attribute

f8f77ad

Removed the placeholder element, and added spacing to body

bc1cbf4

Fixed check-types for travis

ee2091e

Fixed precommit errors

dcf6419

Calculated the body's top before applying ours

6eb14da

Made requested changes

a1454f6

torch2424 force-pushed the sidebar-v1-placeholder branch from e161be4 to a1454f6 Compare June 27, 2017 18:07

torch2424 added 2 commits June 27, 2017 11:18

Made some more offline changes, need a body top test

58adc53

Added tests for sidebar body top

54a0771

camelburrito reviewed Jun 27, 2017

View reviewed changes

camelburrito approved these changes Jun 27, 2017

View reviewed changes

camelburrito merged commit 219742f into ampproject:master Jun 27, 2017

torch2424 mentioned this pull request Jun 27, 2017

Sidebar v1 placeholder manual test torch2424/amphtml#9

Closed

torch2424 deleted the sidebar-v1-placeholder branch October 26, 2018 18:10

[amp-sidebar 1.0][Toolbar] Added Body Spacing for Default Toolbar #10100

[amp-sidebar 1.0][Toolbar] Added Body Spacing for Default Toolbar #10100

Conversation

torch2424 commented Jun 22, 2017 • edited

camelburrito commented Jun 22, 2017

torch2424 commented Jun 23, 2017 • edited

Choose a reason for hiding this comment

torch2424 Jun 23, 2017 • edited

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

torch2424 Jun 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torch2424 Jun 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torch2424 Jun 23, 2017 • edited

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

camelburrito commented Jun 23, 2017

torch2424 commented Jun 24, 2017 • edited

torch2424 commented Jun 27, 2017

torch2424 commented Jun 27, 2017

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

torch2424 commented Jun 22, 2017 •

edited

torch2424 commented Jun 23, 2017 •

edited

torch2424 Jun 23, 2017 •

edited

torch2424 Jun 23, 2017 •

edited

torch2424 Jun 23, 2017 •

edited

torch2424 Jun 23, 2017 •

edited

torch2424 commented Jun 24, 2017 •

edited