Settings: Implement sub-subpages, and clean up some animations.

Settings: Implement sub-subpages, and clean up some animations.

BUG=425627
Committed: https://crrev.com/e48b5c6fb61db5179c85c4811e7ef1936c89d704
Cr-Commit-Position: refs/heads/master@{#343524}

[tommycli, 2015-08-12 17:36:36]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-12 17:36:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1290583002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1290583002/40001

[tommycli, 2015-08-12 17:37:23]

tommycli@chromium.org changed reviewers:
+ dbeam@chromium.org

[tommycli, 2015-08-12 17:37:24]

dbeam: ptal

[commit-bot: I haz the power, 2015-08-12 18:24:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-12 18:24:30]

Dry run: This issue passed the CQ dry run.

[tommycli, 2015-08-12 19:15:19]

a followup question

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animations.js
(left):

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animations.js:22:
timing.duration = 150;
Do we are about the specific timing? If we are fine with the default timings
provided by Polymer, we can delete this file. Otherwise, we need to keep it to
use the 150ms timing...

[Dan Beam, 2015-08-12 21:38:04]

so does this look visually the same as before...?

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.css (right):

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.css:10: margin-top:
10px;
hmmm, why here and not for all subpages?

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:58:
var page = this.history_.slice(-1)[0];
why can't this just be inlined?

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animations.js
(left):

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animations.js:22:
timing.duration = 150;
On 2015/08/12 19:15:19, tommycli wrote:
> Do we are about the specific timing? If we are fine with the default timings
> provided by Polymer, we can delete this file. Otherwise, we need to keep it to
> use the 150ms timing...

we may care.

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.css
(right):

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:14:
margin-bottom: 16px;
alpha sort

[tommycli, 2015-08-12 22:11:57]

dbeam: thanks

Yes, this keeps all the appearances the same.

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.css (right):

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.css:10: margin-top:
10px;
On 2015/08/12 21:38:04, Dan Beam wrote:
> hmmm, why here and not for all subpages?

clarified the css.

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:58:
var page = this.history_.slice(-1)[0];
On 2015/08/12 21:38:04, Dan Beam wrote:
> why can't this just be inlined?

Done.

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animations.js
(left):

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animations.js:22:
timing.duration = 150;
On 2015/08/12 21:38:04, Dan Beam wrote:
> On 2015/08/12 19:15:19, tommycli wrote:
> > Do we are about the specific timing? If we are fine with the default timings
> > provided by Polymer, we can delete this file. Otherwise, we need to keep it
to
> > use the 150ms timing...
> 
> we may care.

Okay. I undeleted this file then.

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_section.css
(right):

https://codereview.chromium.org/1290583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_section.css:14:
margin-bottom: 16px;
On 2015/08/12 21:38:04, Dan Beam wrote:
> alpha sort

Done.

[tommycli, 2015-08-12 22:12:06]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-12 22:12:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1290583002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1290583002/60001

[commit-bot: I haz the power, 2015-08-12 23:23:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-12 23:23:07]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2015-08-13 21:52:10]

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.css (right):

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.css:9: cr-button
.search-engines-subpage {
uh, what?  there's a subpage living inside a button?

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.html (right):

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:40:
on-click="searchEnginesSubpage_" raised>
onSearchEngineSubpageClick_

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:40:
on-click="searchEnginesSubpage_" raised>
should this be on-tap?

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:46:
<paper-icon-button icon="arrow-back" on-tap="handleBack_">
why handleBack vs something like onBack_ or onBackTap_ or onBackClick_?

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animations.js
(right):

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animations.js:17:
configure: function(config) {
can you just customize polymer animations?

[tommycli, 2015-08-13 22:46:47]

dbeam: thx

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.css (right):

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.css:9: cr-button
.search-engines-subpage {
On 2015/08/13 21:52:10, Dan Beam wrote:
> uh, what?  there's a subpage living inside a button?

Done.

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.html (right):

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:40:
on-click="searchEnginesSubpage_" raised>
On 2015/08/13 21:52:10, Dan Beam wrote:
> should this be on-tap?

Done.

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:40:
on-click="searchEnginesSubpage_" raised>
On 2015/08/13 21:52:10, Dan Beam wrote:
> onSearchEngineSubpageClick_

Done.

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.html:46:
<paper-icon-button icon="arrow-back" on-tap="handleBack_">
On 2015/08/13 21:52:10, Dan Beam wrote:
> why handleBack vs something like onBack_ or onBackTap_ or onBackClick_?

Done.

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animations.js
(right):

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animations.js:17:
configure: function(config) {
On 2015/08/13 21:52:10, Dan Beam wrote:
> can you just customize polymer animations?

I don't think so.

I haven't found an easy way to do this. Do you know if we can inherit the
polymer elements declared in this style? I know we can inherit Behaviors, but
behaviors are declared as 'Polymer.NeonAnimationBehavior ='.

We can't easily pass in the timing either. Animations declared on the controller
(neon-animated-pages) only take a name parameter. Animations declared on the
animated items (the pages themselves) can take a config. However, I never was
able to get those animations to work correctly without visual artifacts.

[Dan Beam, 2015-08-14 21:23:07]

lgtm

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animations.js
(right):

https://codereview.chromium.org/1290583002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animations.js:17:
configure: function(config) {
On 2015/08/13 22:46:47, tommycli wrote:
> On 2015/08/13 21:52:10, Dan Beam wrote:
> > can you just customize polymer animations?
> 
> I don't think so.
> 
> I haven't found an easy way to do this. Do you know if we can inherit the
> polymer elements declared in this style? I know we can inherit Behaviors, but
> behaviors are declared as 'Polymer.NeonAnimationBehavior ='.
> 
> We can't easily pass in the timing either. Animations declared on the
controller
> (neon-animated-pages) only take a name parameter. Animations declared on the
> animated items (the pages themselves) can take a config. However, I never was
> able to get those animations to work correctly without visual artifacts.

pretty sure we can, it just may require sending a pull request to polymer

https://codereview.chromium.org/1290583002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.css (right):

https://codereview.chromium.org/1290583002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.css:9:
.search-engines-subpage-button {
did you mean...

  cr-button.search-engines-subpage

(as in: <button class="search-engines-subpage">?)

either way, .search-engines-subpage is a bad class name because it sounds like
it's styling... a subpage

[tommycli, 2015-08-14 21:27:42]

https://codereview.chromium.org/1290583002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.css (right):

https://codereview.chromium.org/1290583002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.css:9:
.search-engines-subpage-button {
On 2015/08/14 21:23:07, Dan Beam wrote:
> did you mean...
> 
>   cr-button.search-engines-subpage
> 
> (as in: <button class="search-engines-subpage">?)
> 
> either way, .search-engines-subpage is a bad class name because it sounds like
> it's styling... a subpage

Hmm... that's what I had in PS#4, yet it was still kind of confusing.

Do you have a recommended class name for the button and the sub-subpage?

[Dan Beam, 2015-08-14 21:33:39]

https://codereview.chromium.org/1290583002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/search_page/search_page.css (right):

https://codereview.chromium.org/1290583002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_page/search_page.css:9:
.search-engines-subpage-button {
On 2015/08/14 21:27:42, tommycli wrote:
> On 2015/08/14 21:23:07, Dan Beam wrote:
> > did you mean...
> > 
> >   cr-button.search-engines-subpage
> > 
> > (as in: <button class="search-engines-subpage">?)
> > 
> > either way, .search-engines-subpage is a bad class name because it sounds
like
> > it's styling... a subpage
> 
> Hmm... that's what I had in PS#4, yet it was still kind of confusing.

no, it's not.  notice the difference between "a.b" and "a .b"

> 
> Do you have a recommended class name for the button and the sub-subpage?

using a compound selector if necessary
1 for presentation (e.g. .subpage or cr-button)
1 for identification (#show-advanced or #show-subpage)

unless there's a subpage with a title of "show" this isn't ambiguous

[tommycli, 2015-08-14 22:30:00]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-14 22:30:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1290583002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1290583002/100001

[tommycli, 2015-08-14 22:49:55]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2015-08-14 22:49:55]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2015-08-14 22:49:55]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1290583002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-08-14 22:50:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1290583002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1290583002/100001

[commit-bot: I haz the power, 2015-08-14 23:23:27]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-08-14 23:24:01]

Patchset 6 (id:??) landed as
https://crrev.com/e48b5c6fb61db5179c85c4811e7ef1936c89d704
Cr-Commit-Position: refs/heads/master@{#343524}