[Search] Make the text swoosh away off the screen when the user taps the opt-in link.

[Search] Make the text swoosh away off the screen when the user taps the opt-in link.

Previously, nothing would happen. Now there is a transition, everything slides away to the bottom of the screen and the rest is loaded.

BUG=402017
TEST=manual
R=huangs@chromium.org, pedrosimonetti@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288974

[huangs, 2014-08-11 16:10:52]

Some comments that needs testing.

https://chromiumcodereview.appspot.com/456833003/diff/1/chrome/browser/resour...
File chrome/browser/resources/contextual_search/promo.css (right):

https://chromiumcodereview.appspot.com/456833003/diff/1/chrome/browser/resour...
chrome/browser/resources/contextual_search/promo.css:43: -webkit-transform:
none;
Probably redundant. Same below.

https://chromiumcodereview.appspot.com/456833003/diff/1/chrome/browser/resour...
File chrome/browser/resources/contextual_search/promo.html (right):

https://chromiumcodereview.appspot.com/456833003/diff/1/chrome/browser/resour...
chrome/browser/resources/contextual_search/promo.html:19: <span class="label"
onclick="addSlideClass()"><a class="colored" href="#optin"
i18n-content="optIn"></a></span>
Better style is to inject handler from JS, since in this case you won't need to
have global function as handler.  Though now you'll have to grab label somehow.

https://chromiumcodereview.appspot.com/456833003/diff/1/chrome/browser/resour...
File chrome/browser/resources/contextual_search/promo.js (right):

https://chromiumcodereview.appspot.com/456833003/diff/1/chrome/browser/resour...
chrome/browser/resources/contextual_search/promo.js:10: $('container').className
+= ' slide';
Is it standard to use $()?

Use .classList.add('slide'), same below.

[pedro (no code reviews), 2014-08-11 17:54:06]

https://codereview.chromium.org/456833003/diff/1/chrome/browser/resources/con...
File chrome/browser/resources/contextual_search/promo.css (right):

https://codereview.chromium.org/456833003/diff/1/chrome/browser/resources/con...
chrome/browser/resources/contextual_search/promo.css:43: -webkit-transform:
none;
No need for transforms here. Animating the container will animate the buttons
too.

https://codereview.chromium.org/456833003/diff/1/chrome/browser/resources/con...
chrome/browser/resources/contextual_search/promo.css:56: -webkit-transform:
translate(0, 1000px);
Let's talk about the animation offline if you want, but I think we should use a
slide down animation. Quantum animations are coming to the Serp and the results
will fade in + slide up when revealing, so having a slide down animation
happening before that sounds counter intuitive.

I think the animation should be a simple fade out, or a fade out + translateZ
(moving away from the user).

https://codereview.chromium.org/456833003/diff/1/chrome/browser/resources/con...
File chrome/browser/resources/contextual_search/promo.html (right):

https://codereview.chromium.org/456833003/diff/1/chrome/browser/resources/con...
chrome/browser/resources/contextual_search/promo.html:19: <span class="label"
onclick="addSlideClass()"><a class="colored" href="#optin"
i18n-content="optIn"></a></span>
On 2014/08/11 16:10:51, huangs1 wrote:
> Better style is to inject handler from JS, since in this case you won't need
to
> have global function as handler.  Though now you'll have to grab label
somehow.

+1

[Mathieu, 2014-08-11 21:11:04]

Thanks, have a look!

https://codereview.chromium.org/456833003/diff/1/chrome/browser/resources/con...
File chrome/browser/resources/contextual_search/promo.css (right):

https://codereview.chromium.org/456833003/diff/1/chrome/browser/resources/con...
chrome/browser/resources/contextual_search/promo.css:43: -webkit-transform:
none;
On 2014/08/11 17:54:06, pedrosimonetti wrote:
> No need for transforms here. Animating the container will animate the buttons
> too.

Acknowledged.

https://codereview.chromium.org/456833003/diff/1/chrome/browser/resources/con...
chrome/browser/resources/contextual_search/promo.css:43: -webkit-transform:
none;
On 2014/08/11 16:10:51, huangs1 wrote:
> Probably redundant. Same below.

Acknowledged.

https://codereview.chromium.org/456833003/diff/1/chrome/browser/resources/con...
chrome/browser/resources/contextual_search/promo.css:56: -webkit-transform:
translate(0, 1000px);
On 2014/08/11 17:54:06, pedrosimonetti wrote:
> Let's talk about the animation offline if you want, but I think we should use
a
> slide down animation. Quantum animations are coming to the Serp and the
results
> will fade in + slide up when revealing, so having a slide down animation
> happening before that sounds counter intuitive.
> 
> I think the animation should be a simple fade out, or a fade out + translateZ
> (moving away from the user).

Acknowledged.

https://codereview.chromium.org/456833003/diff/1/chrome/browser/resources/con...
File chrome/browser/resources/contextual_search/promo.html (right):

https://codereview.chromium.org/456833003/diff/1/chrome/browser/resources/con...
chrome/browser/resources/contextual_search/promo.html:19: <span class="label"
onclick="addSlideClass()"><a class="colored" href="#optin"
i18n-content="optIn"></a></span>
On 2014/08/11 17:54:06, pedrosimonetti wrote:
> On 2014/08/11 16:10:51, huangs1 wrote:
> > Better style is to inject handler from JS, since in this case you won't need
> to
> > have global function as handler.  Though now you'll have to grab label
> somehow.
> 
> +1

Done.

https://codereview.chromium.org/456833003/diff/1/chrome/browser/resources/con...
chrome/browser/resources/contextual_search/promo.html:19: <span class="label"
onclick="addSlideClass()"><a class="colored" href="#optin"
i18n-content="optIn"></a></span>
On 2014/08/11 16:10:51, huangs1 wrote:
> Better style is to inject handler from JS, since in this case you won't need
to
> have global function as handler.  Though now you'll have to grab label
somehow.

Done.

https://codereview.chromium.org/456833003/diff/1/chrome/browser/resources/con...
File chrome/browser/resources/contextual_search/promo.js (right):

https://codereview.chromium.org/456833003/diff/1/chrome/browser/resources/con...
chrome/browser/resources/contextual_search/promo.js:10: $('container').className
+= ' slide';
On 2014/08/11 16:10:51, huangs1 wrote:
> Is it standard to use $()?
> 
> Use .classList.add('slide'), same below.

In Chrome files yes, there is a presubmit to use $

[pedro (no code reviews), 2014-08-11 21:38:02]

lgtm

LGTM with a couple of nits

https://codereview.chromium.org/456833003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/contextual_search/promo.css (right):

https://codereview.chromium.org/456833003/diff/20001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.css:51: opacity: 0;
Nit: prefixed properties (-webkit) should come first.

https://codereview.chromium.org/456833003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/contextual_search/promo.js (right):

https://codereview.chromium.org/456833003/diff/20001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.js:12:
$('container').classList.add('fadeout');
Nit: if you use names describing the function (hide/dismiss) instead of the
implementation (slide/fadeout) you won't have to rename it every time the
animation specs change :)

[Mathieu, 2014-08-12 12:39:47]

Thanks!

https://codereview.chromium.org/456833003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/contextual_search/promo.css (right):

https://codereview.chromium.org/456833003/diff/20001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.css:51: opacity: 0;
On 2014/08/11 21:38:02, pedrosimonetti wrote:
> Nit: prefixed properties (-webkit) should come first.

Done.

https://codereview.chromium.org/456833003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/contextual_search/promo.js (right):

https://codereview.chromium.org/456833003/diff/20001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.js:12:
$('container').classList.add('fadeout');
On 2014/08/11 21:38:02, pedrosimonetti wrote:
> Nit: if you use names describing the function (hide/dismiss) instead of the
> implementation (slide/fadeout) you won't have to rename it every time the
> animation specs change :)

Done.

[huangs, 2014-08-12 15:17:28]

LGTM with comment on comment.

https://codereview.chromium.org/456833003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/contextual_search/promo.js (right):

https://codereview.chromium.org/456833003/diff/40001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.js:7: * On load, registers the
handler to add the 'fadeout' class to the container
Update comment re. 'fadeout', and implementation?

[Mathieu, 2014-08-12 15:18:45]

Committed patchset #4 manually as 288974 (presubmit successful).

[Mathieu, 2014-08-12 15:19:06]

https://codereview.chromium.org/456833003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/contextual_search/promo.js (right):

https://codereview.chromium.org/456833003/diff/40001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.js:7: * On load, registers the
handler to add the 'fadeout' class to the container
On 2014/08/12 15:17:28, huangs1 wrote:
> Update comment re. 'fadeout', and implementation?

Done.