[MD settings] layout of site settings sub-pages

[MD settings] layout of site settings sub-pages

This CL changes the layout in the sub-pages of site-settings.

BUG=599667, 589630
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/221cf0f7b4e856a41992494ce0ef04448f41661f
Cr-Commit-Position: refs/heads/master@{#393053}

[dschuyler, 2016-05-10 01:31:57]

Description was changed from

==========
[MD settings] layout of site settings sub-pages

This CL changes the layout in the sub-pages of site-settings.

BUG=599667, 589630
==========

to

==========
[MD settings] layout of site settings sub-pages

This CL changes the layout in the sub-pages of site-settings.

BUG=599667, 589630
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2016-05-10 01:34:16]

Patchset #1 (id:1) has been deleted

[dschuyler, 2016-05-10 01:34:49]

dschuyler@chromium.org changed reviewers:
+ finnur@chromium.org

[Finnur, 2016-05-10 13:05:22]

Looks like you need to make some changes to get the tests to pass, but here are
some comments in the meantime.

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.css (left):

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.css:6:
-webkit-margin-start: 7px;
This was to make sure the icon was in line vertically with the back arrow, as
per issue 608131.

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.html (left):

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.html:37:
<paper-item-body two-line on-tap="onOriginTap_" class="origin">
The origin style was meant to separate the icon a bit from the origin, as per
mocks, I believe. Is the removal intentional?

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.html (right):

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.html:22: }
Why is this here and not in the (newly removed) .css file? Is this a new
convention -- to have styles appear in the html file, as opposed to on their
own?

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.html:33: <div
class="list-frame menu-content vertical-list" id="listContainer">
The list-frame class adds a lot of left padding to the list. I'm assuming this
is to be consistent with the all other lists for md-settings (as opposed to
following some mocks)?

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_category.html
(left):

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.html:26:
<div>[[computeTitleForContentCategory(category)]]</div>
Umm... this isn't right. The icon and title is now missing...

[dschuyler, 2016-05-10 20:44:14]

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.css (left):

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.css:6:
-webkit-margin-start: 7px;
On 2016/05/10 13:05:22, Finnur wrote:
> This was to make sure the icon was in line vertically with the back arrow, as
> per issue 608131.

That alignment is being done a different way. So this adjustment
can be removed.

https://codereview.chromium.org/1957823002/#ps80001

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.html (left):

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.html:37:
<paper-item-body two-line on-tap="onOriginTap_" class="origin">
On 2016/05/10 13:05:22, Finnur wrote:
> The origin style was meant to separate the icon a bit from the origin, as per
> mocks, I believe. Is the removal intentional?

Thanks, I thought the settings-shared had this done correctly.
It turns out I needed an adjustment in .middle.

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_list.html (right):

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.html:22: }
On 2016/05/10 13:05:22, Finnur wrote:
> Why is this here and not in the (newly removed) .css file? Is this a new
> convention -- to have styles appear in the html file, as opposed to on their
> own?

Yep, it's the new style of style. I made some notes about it here:
https://docs.google.com/document/d/1G0t519y_cSW4etXVwruR145abZH0N5-9R7JyejzVo...

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_list.html:33: <div
class="list-frame menu-content vertical-list" id="listContainer">
On 2016/05/10 13:05:22, Finnur wrote:
> The list-frame class adds a lot of left padding to the list. I'm assuming this
> is to be consistent with the all other lists for md-settings (as opposed to
> following some mocks)?

Both, it's about having the lists be consistent and also the mocks
call for a 56px margin. (More literally, the mocks call for a 20px +
20px + 16px margin).

https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Inner%20Pag...

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_category.html
(left):

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.html:26:
<div>[[computeTitleForContentCategory(category)]]</div>
On 2016/05/10 13:05:22, Finnur wrote:
> Umm... this isn't right. The icon and title is now missing...

The title and icon are not present in the mocks.

https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Inner%20Pag...

[Finnur, 2016-05-11 10:05:57]

LGTM

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_category.html
(left):

https://codereview.chromium.org/1963013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.html:26:
<div>[[computeTitleForContentCategory(category)]]</div>
Interesting... 

The title was redundant, I guess, but I kind of miss the icon, though...

Well, I guess it is easy to add back if someone complains. :)

[dschuyler, 2016-05-11 16:51:58]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2016-05-11 16:52:15]

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

[commit-bot: I haz the power, 2016-05-11 16:56:19]

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

[commit-bot: I haz the power, 2016-05-11 16:56:20]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dschuyler, 2016-05-11 20:43:11]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2016-05-11 20:43:11]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/1963013002/#ps60001
(title: "merge with master")

[commit-bot: I haz the power, 2016-05-11 20:43:47]

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

[commit-bot: I haz the power, 2016-05-11 20:50:36]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-05-11 20:52:12]

Description was changed from

==========
[MD settings] layout of site settings sub-pages

This CL changes the layout in the sub-pages of site-settings.

BUG=599667, 589630
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] layout of site settings sub-pages

This CL changes the layout in the sub-pages of site-settings.

BUG=599667, 589630
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/221cf0f7b4e856a41992494ce0ef04448f41661f
Cr-Commit-Position: refs/heads/master@{#393053}
==========

[commit-bot: I haz the power, 2016-05-11 20:52:13]

Patchset 3 (id:??) landed as
https://crrev.com/221cf0f7b4e856a41992494ce0ef04448f41661f
Cr-Commit-Position: refs/heads/master@{#393053}