MD Settings: Fix several global UI polish bugs.

MD Settings: Fix several global UI polish bugs.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2656563006
Cr-Commit-Position: refs/heads/master@{#447310}
Committed: https://chromium.googlesource.com/chromium/src/+/a2176d0c08e0a2367c91c2edb8ae255f185130ae

[scottchen, 2017-01-25 19:20:05]

Description was changed from

==========
MD Settings: Fix various Pri-1 visual bugs.

Designer Alan Bettes combed through md-settings and made a list of visual bugs.
This CL aims to fix all the Pri-1 bugs for M58 launch.

BUG=684152
==========

to

==========
MD Settings: Fix various Pri-1 visual bugs.

Designer Alan Bettes combed through md-settings and made a list of visual bugs.
This CL aims to fix all the Pri-1 bugs for M58 launch.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[scottchen, 2017-01-25 19:25:18]

Description was changed from

==========
MD Settings: Fix various Pri-1 visual bugs.

Designer Alan Bettes combed through md-settings and made a list of visual bugs.
This CL aims to fix all the Pri-1 bugs for M58 launch.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix various Pri-1 visual bugs.

bettes@ combed through md-settings and made a list of visual bugs. This CL aims
to fix all the Pri-1 bugs for M58 launch.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[scottchen, 2017-01-25 19:28:08]

Description was changed from

==========
MD Settings: Fix various Pri-1 visual bugs.

bettes@ combed through md-settings and made a list of visual bugs. This CL aims
to fix all the Pri-1 bugs for M58 launch.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix several UI polish bugs.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[scottchen, 2017-01-25 21:51:05]

Description was changed from

==========
MD Settings: Fix several UI polish bugs.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix several global UI polish bugs.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[scottchen, 2017-01-26 00:15:00]

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

[scottchen, 2017-01-26 00:16:20]

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

[commit-bot: I haz the power, 2017-01-26 00:17:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 01:52:15]

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

[commit-bot: I haz the power, 2017-01-26 01:52:16]

Dry run: This issue passed the CQ dry run.

[scottchen, 2017-01-26 23:17:49]

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

[commit-bot: I haz the power, 2017-01-26 23:18:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-01-27 00:30:31]

The code looks okay. Please add a screen shot of the new look (maybe in the
bug).

[commit-bot: I haz the power, 2017-01-27 02:19:39]

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

[commit-bot: I haz the power, 2017-01-27 02:19:40]

Dry run: This issue passed the CQ dry run.

[scottchen, 2017-01-27 21:55:56]

Patchset #5 (id:80001) has been deleted

[scottchen, 2017-01-28 01:31:01]

On 2017/01/27 00:30:31, dschuyler wrote:
> The code looks okay. Please add a screen shot of the new look (maybe in the
> bug).

@dschuyler http://imgur.com/a/RSdyy showcases all the changes.

[dschuyler, 2017-01-28 02:24:26]

Thanks for the screen shot, that really helped. Now I have some other comments
:)

https://codereview.chromium.org/2656563006/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/people_page.html (right):

https://codereview.chromium.org/2656563006/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/people_page.html:86: min-height:
85px;
How about making this var(--settings-row-three-line-min-height) after adding
--settings-row-three-line-min-height: 85px; to settings_vars_css.html

https://codereview.chromium.org/2656563006/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/people_page.html:91: max-width:
460px;
Can this be done in a way that will allow the card/box with to change (without
this value needing to be updated)? Maybe using a percentage (maybe something
else).

https://codereview.chromium.org/2656563006/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_subpage.html
(right):

https://codereview.chromium.org/2656563006/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_subpage.html:21:
padding-bottom: 24px;
It may be an optical illusion, but the screen shot of this padding looks much
larger than in Alan's reference screen shot. I'm wondering if Alan meant from
the bottom of one line of text to the top of the other line of text should be
24px (that's the way the mock have often shown spacing). Because the text lines
themselves may have space above/below them, the 24px may need to be a smaller
value to get the same visual result (or the other spacing needs to be removed).

https://codereview.chromium.org/2656563006/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_vars_css.html (left):

https://codereview.chromium.org/2656563006/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_vars_css.html:48: margin-top: 4px;
Just so I know if I'm following this correctly, this is being superseded by
line-height: 154%; /* Apply 20px line-height to all texts by default. */
in this same CL?

[scottchen, 2017-01-30 22:39:28]

https://codereview.chromium.org/2656563006/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/people_page.html (right):

https://codereview.chromium.org/2656563006/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/people_page.html:86: min-height:
85px;
On 2017/01/28 02:24:26, dschuyler wrote:
> How about making this var(--settings-row-three-line-min-height) after adding
> --settings-row-three-line-min-height: 85px; to settings_vars_css.html

I like that idea, done.

https://codereview.chromium.org/2656563006/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/people_page.html:91: max-width:
460px;
On 2017/01/28 02:24:26, dschuyler wrote:
> Can this be done in a way that will allow the card/box with to change (without
> this value needing to be updated)? Maybe using a percentage (maybe something
> else).

Done.

https://codereview.chromium.org/2656563006/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_subpage.html
(right):

https://codereview.chromium.org/2656563006/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_subpage.html:21:
padding-bottom: 24px;
On 2017/01/28 02:24:26, dschuyler wrote:
> It may be an optical illusion, but the screen shot of this padding looks much
> larger than in Alan's reference screen shot. I'm wondering if Alan meant from
> the bottom of one line of text to the top of the other line of text should be
> 24px (that's the way the mock have often shown spacing). Because the text
lines
> themselves may have space above/below them, the 24px may need to be a smaller
> value to get the same visual result (or the other spacing needs to be
removed).


I think I misunderstood the bug report for this one.

https://codereview.chromium.org/2656563006/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_vars_css.html (left):

https://codereview.chromium.org/2656563006/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_vars_css.html:48: margin-top: 4px;
On 2017/01/28 02:24:26, dschuyler wrote:
> Just so I know if I'm following this correctly, this is being superseded by
> line-height: 154%; /* Apply 20px line-height to all texts by default. */
> in this same CL?

You're correct!

[dschuyler, 2017-01-30 22:48:12]

lgtm

https://codereview.chromium.org/2656563006/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_subpage.html
(right):

https://codereview.chromium.org/2656563006/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_subpage.html:38:
font-weight: 500;
nit: since there's only the one h1 here, would it make sense to bring line 25
(-webkit-margin-start: 4px;) down here and remove the case above?

[scottchen, 2017-01-30 23:05:36]

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

[commit-bot: I haz the power, 2017-01-30 23:06:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottchen, 2017-01-31 00:31:57]

The CQ bit was unchecked by scottchen@chromium.org

[scottchen, 2017-01-31 00:32:02]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-01-31 00:33:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 00:43:30]

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

[commit-bot: I haz the power, 2017-01-31 00:43:31]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[scottchen, 2017-01-31 01:03:10]

scottchen@chromium.org changed reviewers:
+ michaelpg@chromium.org

[scottchen, 2017-01-31 01:03:12]

+ michaelpg@, needs owner as per presubmit check.

[Dan Beam, 2017-01-31 03:02:24]

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

[Dan Beam, 2017-01-31 03:02:24]

https://codereview.chromium.org/2656563006/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right):

https://codereview.chromium.org/2656563006/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:14: 0px 16px 16px
rgba(0, 0, 0, 0.24);
i highly suspect we're copying this from somewhere, but .... fine, lgtm

[scottchen, 2017-01-31 20:03:57]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-01-31 20:04:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 20:10:03]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485893037464710,
"parent_rev": "b6f1374b024b68b909ffb632b5a8c184391476bf", "commit_rev":
"a2176d0c08e0a2367c91c2edb8ae255f185130ae"}

[commit-bot: I haz the power, 2017-01-31 20:10:40]

Description was changed from

==========
MD Settings: Fix several global UI polish bugs.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix several global UI polish bugs.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2656563006
Cr-Commit-Position: refs/heads/master@{#447310}
Committed:
https://chromium.googlesource.com/chromium/src/+/a2176d0c08e0a2367c91c2edb8ae...
==========

[commit-bot: I haz the power, 2017-01-31 20:10:42]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/a2176d0c08e0a2367c91c2edb8ae...