Add snaps property to app-header
#99
Conversation
blasten
force-pushed
the
header-snap
branch
2 times, most recently
from
f08dafc
to
b14ea1e
Compare
keanulee
changed the title
snaps property to app-headersnaps property to app-header
blasten
force-pushed
the
header-snap
branch
6 times, most recently
from
4c11624
to
6228144
Compare
blasten
changed the title
snaps property to app-headersnaps property to app-header
| @@ -381,9 +407,8 @@ | |||
| this._layoutIfDirty(); | |||
| } | |||
| // restore no transition | |||
| this.disabled = savedDisabled; | |||
| this.disabled = false; | |||
There was a problem hiding this comment.
Did you mean to remove savedDisabled? If so, remove that variable?
|
Also, I just tested everything in IOWA. |
| */ | ||
| snaps: { | ||
| type: Boolean, | ||
| reflectToAttribute: true, |
There was a problem hiding this comment.
Does this need reflectToAttribute? I don't see any style rules for [snaps]...
| @@ -52,7 +53,7 @@ | |||
| <!-- default to use body scroller --> | |||
| <app-header-layout> | |||
|
|
|||
| <app-header condenses reveals> | |||
| <app-header effects="waterfall" condenses reveals> | |||
| <app-toolbar> | |||
| <paper-icon-button icon="menu"></paper-icon-button> | |||
| <div class="flex"></div> | |||
There was a problem hiding this comment.
Can you remove using layout-classes in the demo? Thanks.
blasten
force-pushed
the
header-snap
branch
9 times, most recently
from
3acad08
to
653adf1
Compare
|
@frankiefu I just fixed the issues. |
| @@ -95,6 +95,8 @@ | |||
| position: relative; | |||
| display: block; | |||
| -webkit-transform: translateZ(0); | |||
There was a problem hiding this comment.
Is there supposed to be the unprefixed version of transform as well? There should also be a transition-property: -webkit-transform.
There was a problem hiding this comment.
Also, before -webkit-transform:
transform: translateZ(0);There was a problem hiding this comment.
that was added to fix an issue in Safari related to the waterfall shadow, but it's unfortunate that I didn't leave a comment.
blasten
force-pushed
the
header-snap
branch
3 times, most recently
from
f2d13d2
to
1e18489
Compare
| @@ -95,6 +95,9 @@ | |||
| position: relative; | |||
| display: block; | |||
| -webkit-transform: translateZ(0); | |||
| transition-property: -webkit-transform; | |||
| transition-property: transform; | |||
There was a problem hiding this comment.
The 2nd transition-propertyoverwrites the 1st transition-property.
There was a problem hiding this comment.
I don't think it'll overwrite if the second one is not recognized (i.e. if the browser does not support the unprefixed transform, it'll ignore the second one). Obviously we do want it to override if it is supported.
There was a problem hiding this comment.
@frankiefu We do a similar thing in app-drawer https://github.com/PolymerLabs/app-layout/blob/master/app-drawer/app-drawer.html#L92
There was a problem hiding this comment.
The second one will overwrite the first one since both are setting on the same CSS property (transition-property). If you need to support prefix one it needs to use the prefix transition, e.g.
-webkit-transition-property: -webkit-transform;
transition-property: transform;
There was a problem hiding this comment.
Btw, only Safari 8 needs -webkit prefix for transform. So I am guessing you will see the problem when running on Safari 8.
|
LGTM |
Add `snaps` property to app-header
snapsproperty for app-header.I added it to the shrine template and it looks much better. cc @keanulee
Small breaking change: the event
app-header-transformis gone and should be replaced by adding a scroll event listeners to the header's scroll target.