MD Settings: Adding "About" page basic structure.

MD Settings: Adding "About" page basic structure.

BUG=603625
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/45ab5fa295d517c842053780bed69ee5d88f382b
Cr-Commit-Position: refs/heads/master@{#392162}

[dpapad, 2016-05-05 21:51:34]

Description was changed from

==========
MD Settings: Adding about page basic structure.

BUG=603625
==========

to

==========
MD Settings: Adding about page basic structure.

BUG=603625
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-05-05 22:46:33]

Description was changed from

==========
MD Settings: Adding about page basic structure.

BUG=603625
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Adding "About" page basic structure.

BUG=603625
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-05-05 23:06:26]

Patchset #2 (id:20001) has been deleted

[dpapad, 2016-05-05 23:06:30]

Patchset #1 (id:1) has been deleted

[dpapad, 2016-05-05 23:19:14]

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

[dpapad, 2016-05-05 23:19:15]

Screenshot at http://imgur.com/El98wIg.

The page is still a "dummy", not displaying any real data. There is plenty of
work left, which will be addressed on follow up CLs (C++ handler, browser proxy,
tests, polish, ChromeOS etc).

Thanks,
Demetrios

[tommycli, 2016-05-06 18:01:44]

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

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:4: <link
rel="import"
href="chrome://resources/polymer/v1_0/paper-button/paper-button.html">
Do you also need to import main_page_behavior?

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:11: .logo-container
{
Which is the element with the logo-container class?

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:13: }
\n between entries in css

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:17: margin-bottom:
auto;
I assume these auto margins are to center? Does flexbox's align-items or
align-content also work? (If your container is already a flexbox that is) I find
that more explicit.

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:24:
current-route="[[currentRoute]]" section="">
Since Michael rewrote settings-section animations, i'm not sure what the effect
of section="" is. I'm okay with it if it seems to work., this is just a warning
that I'm not sure what the behavior is now.

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:24:
current-route="[[currentRoute]]" section="">
Additionally, I'm not sure if current-route="[[currentRoute]]" is needed
anymore. Looking at the source, it kind of seems that settings-section has been
made "dumb" and no longer needs the route. (And main_page_behavior handles
that). Just a nit.

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:34: <div>Your
device is up to date</div>
If there are mocks and these are the final English strings, it would be nice if
there were as few un-internationalized strings within the committed patches as
possible.

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:1: // Copyright 2015
The Chromium Authors. All rights reserved.
2016

[tommycli, 2016-05-06 18:02:30]

On 2016/05/06 18:01:44, tommycli wrote:
>
https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/settings/about_page/about_page.html (right):
> 
>
https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/about_page/about_page.html:4: <link
> rel="import"
> href="chrome://resources/polymer/v1_0/paper-button/paper-button.html">
> Do you also need to import main_page_behavior?
> 
>
https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/about_page/about_page.html:11:
.logo-container
> {
> Which is the element with the logo-container class?
> 
>
https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/about_page/about_page.html:13: }
> \n between entries in css
> 
>
https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/about_page/about_page.html:17:
margin-bottom:
> auto;
> I assume these auto margins are to center? Does flexbox's align-items or
> align-content also work? (If your container is already a flexbox that is) I
find
> that more explicit.
> 
>
https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/about_page/about_page.html:24:
> current-route="[[currentRoute]]" section="">
> Since Michael rewrote settings-section animations, i'm not sure what the
effect
> of section="" is. I'm okay with it if it seems to work., this is just a
warning
> that I'm not sure what the behavior is now.
> 
>
https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/about_page/about_page.html:24:
> current-route="[[currentRoute]]" section="">
> Additionally, I'm not sure if current-route="[[currentRoute]]" is needed
> anymore. Looking at the source, it kind of seems that settings-section has
been
> made "dumb" and no longer needs the route. (And main_page_behavior handles
> that). Just a nit.
> 
>
https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/about_page/about_page.html:34: <div>Your
> device is up to date</div>
> If there are mocks and these are the final English strings, it would be nice
if
> there were as few un-internationalized strings within the committed patches as
> possible.
> 
>
https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/settings/about_page/about_page.js (right):
> 
>
https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/about_page/about_page.js:1: // Copyright
2015
> The Chromium Authors. All rights reserved.
> 2016

lgtm except the above (mostly superficial) issues.

[dpapad, 2016-05-06 18:33:29]

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

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:4: <link
rel="import"
href="chrome://resources/polymer/v1_0/paper-button/paper-button.html">
On 2016/05/06 at 18:01:43, tommycli wrote:
> Do you also need to import main_page_behavior?

I looked into this. MainPageBehavior has methods related to opening/closing
subsections. Given that the About page does not have any subsections (unlike
'basic' and 'abvanced') it is not needed.

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:11: .logo-container
{
On 2016/05/06 at 18:01:43, tommycli wrote:
> Which is the element with the logo-container class?

Removed. That was a remainder from a previous state of my code, where I was not
using settings-box yet.

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:13: }
On 2016/05/06 at 18:01:43, tommycli wrote:
> \n between entries in css

Done.

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:17: margin-bottom:
auto;
On 2016/05/06 at 18:01:43, tommycli wrote:
> I assume these auto margins are to center? Does flexbox's align-items or
align-content also work? (If your container is already a flexbox that is) I find
that more explicit.

Removed. You are right, align-items: center, works and settings-box already does
that, so these margins are not needed anymore.

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:24:
current-route="[[currentRoute]]" section="">
On 2016/05/06 at 18:01:43, tommycli wrote:
> Since Michael rewrote settings-section animations, i'm not sure what the
effect of section="" is. I'm okay with it if it seems to work., this is just a
warning that I'm not sure what the behavior is now.

Removed section="" completely. Navigation still seems to work fine.
Regarding current-route, I see that all settings-sections still use it, see
https://code.google.com/p/chromium/codesearch#search/&q=settings-section%20fi....
If it is not needed we can remove it from all <settings-section> instances in a
separate CL.

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:34: <div>Your
device is up to date</div>
On 2016/05/06 at 18:01:43, tommycli wrote:
> If there are mocks and these are the final English strings, it would be nice
if there were as few un-internationalized strings within the committed patches
as possible.

The final string is a combination of dynamic and static data
For Chrome: "Google Chrome is up to date" 
For Chromium: "Chromium is up to date"
For Chrome OS: "Your device is up to date"

I prefer to localize all the strings of this page in a separate CL. I can land
that separate CL before this one or after, don't mind much. Let me know if you
have a preference.

[tommycli, 2016-05-06 18:50:21]

still lgtm

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

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:4: <link
rel="import"
href="chrome://resources/polymer/v1_0/paper-button/paper-button.html">
On 2016/05/06 18:33:29, dpapad wrote:
> On 2016/05/06 at 18:01:43, tommycli wrote:
> > Do you also need to import main_page_behavior?
> 
> I looked into this. MainPageBehavior has methods related to opening/closing
> subsections. Given that the About page does not have any subsections (unlike
> 'basic' and 'abvanced') it is not needed.

I asked because you use main-page-styles below. If it's unneeded, you could just
fold the custom styles into the settings-shared block?

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:34: <div>Your
device is up to date</div>
On 2016/05/06 18:33:29, dpapad wrote:
> On 2016/05/06 at 18:01:43, tommycli wrote:
> > If there are mocks and these are the final English strings, it would be nice
> if there were as few un-internationalized strings within the committed patches
> as possible.
> 
> The final string is a combination of dynamic and static data
> For Chrome: "Google Chrome is up to date" 
> For Chromium: "Chromium is up to date"
> For Chrome OS: "Your device is up to date"
> 
> I prefer to localize all the strings of this page in a separate CL. I can land
> that separate CL before this one or after, don't mind much. Let me know if you
> have a preference.

Ah that's fine then. I'm okay with localizing in separate CL

[dpapad, 2016-05-06 19:01:31]

Thanks!

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

https://codereview.chromium.org/1950423003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.html:4: <link
rel="import"
href="chrome://resources/polymer/v1_0/paper-button/paper-button.html">
On 2016/05/06 at 18:50:20, tommycli wrote:
> On 2016/05/06 18:33:29, dpapad wrote:
> > On 2016/05/06 at 18:01:43, tommycli wrote:
> > > Do you also need to import main_page_behavior?
> > 
> > I looked into this. MainPageBehavior has methods related to opening/closing
> > subsections. Given that the About page does not have any subsections (unlike
> > 'basic' and 'abvanced') it is not needed.
> 
> I asked because you use main-page-styles below. If it's unneeded, you could
just fold the custom styles into the settings-shared block?

Done.

[dpapad, 2016-05-06 20:49:52]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-05-06 20:49:53]

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

[commit-bot: I haz the power, 2016-05-06 20:50:17]

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

[commit-bot: I haz the power, 2016-05-06 21:30:32]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-05-06 21:32:45]

Description was changed from

==========
MD Settings: Adding "About" page basic structure.

BUG=603625
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Adding "About" page basic structure.

BUG=603625
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/45ab5fa295d517c842053780bed69ee5d88f382b
Cr-Commit-Position: refs/heads/master@{#392162}
==========

[commit-bot: I haz the power, 2016-05-06 21:32:46]

Patchset 4 (id:??) landed as
https://crrev.com/45ab5fa295d517c842053780bed69ee5d88f382b
Cr-Commit-Position: refs/heads/master@{#392162}