MD Settings: Fix Network section styling

MD Settings: Fix Network section styling

This CL makes Settings specific changes to the 'Network' (was 'Internet
conneciton') section.

See issue for specs and screenshots.

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

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

[stevenjb, 2017-03-16 20:06:39]

Description was changed from

==========
MD Settings: Fix Network section styling

This CL makes Settings specific changes to the 'Network' (was 'Internet
conneciton') section.

Se issue for specs and screenshots.

BUG=701945
==========

to

==========
MD Settings: Fix Network section styling

This CL makes Settings specific changes to the 'Network' (was 'Internet
conneciton') section.

Se issue for specs and screenshots.

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

[stevenjb, 2017-03-17 00:16:43]

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

[dschuyler, 2017-03-20 18:26:57]

Description was changed from

==========
MD Settings: Fix Network section styling

This CL makes Settings specific changes to the 'Network' (was 'Internet
conneciton') section.

Se issue for specs and screenshots.

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

to

==========
MD Settings: Fix Network section styling

This CL makes Settings specific changes to the 'Network' (was 'Internet
conneciton') section.

See issue for specs and screenshots.

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

[dschuyler, 2017-03-20 19:11:31]

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_detail_page.html
(right):

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:68:
font-size: 14px;
Please use a percentage for font sizes, so that they scale when the default font
size changes. E.g.
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:242:
<div class="flex">$i18n{networkSectionNetwork}</div>
s/flex/start/

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:274:
<div class="flex">$i18n{networkSectionProxy}</div>
s/flex/start/

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_detail_page.js
(right):

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.js:565: if
(event.target.tagName == 'CR-EXPAND-BUTTON')
Now that there are three+ uses of this string constant, let's make a const var
for it.

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/internet_page/internet_known_networks_page.html
(right):

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_known_networks_page.html:24:
<div class="list-frame">
Please add class vertical-list here and line 49; then remove lines 12-13.

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_subpage.js
(right):

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_subpage.js:32: * @type
{?DeviceStateProperties|undefined}
Can this be undefined, or will it either be null or defined?

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_proxy.html (right):

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.html:55: <div
class="settings-box continuation single-column indent">
Can "settings-box continuation single-column indent" be replaced with
"list-frame"?

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:907:
{"networkSectionAdvancedExpandA11yLabel",
Would it make sense to s/Expand// here? I'm not sure, I'm just pattern matching
vs. the IDS_ on line 908.

[stevenjb, 2017-03-24 21:00:03]

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_detail_page.html
(right):

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:68:
font-size: 14px;
On 2017/03/20 19:11:30, dschuyler wrote:
> Please use a percentage for font sizes, so that they scale when the default
font
> size changes. E.g.
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...

Right... done. For the record it would be nice if we could just match specs and
still have fonts scale appropriately.

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:242:
<div class="flex">$i18n{networkSectionNetwork}</div>
On 2017/03/20 19:11:30, dschuyler wrote:
> s/flex/start/

Done.

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:274:
<div class="flex">$i18n{networkSectionProxy}</div>
On 2017/03/20 19:11:30, dschuyler wrote:
> s/flex/start/

Done.

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_detail_page.js
(right):

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.js:565: if
(event.target.tagName == 'CR-EXPAND-BUTTON')
On 2017/03/20 19:11:30, dschuyler wrote:
> Now that there are three+ uses of this string constant, let's make a const var
> for it.

Pretty much every use I could find of tagName uses a raw string. It's the JS
way! I went ahead and defined a local const member, FWIW.

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/internet_page/internet_known_networks_page.html
(right):

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_known_networks_page.html:24:
<div class="list-frame">
On 2017/03/20 19:11:30, dschuyler wrote:
> Please add class vertical-list here and line 49; then remove lines 12-13.

Done.

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_subpage.js
(right):

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_subpage.js:32: * @type
{?DeviceStateProperties|undefined}
On 2017/03/20 19:11:30, dschuyler wrote:
> Can this be undefined, or will it either be null or defined?

Done.

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_proxy.html (right):

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.html:55: <div
class="settings-box continuation single-column indent">
On 2017/03/20 19:11:31, dschuyler wrote:
> Can "settings-box continuation single-column indent" be replaced with
> "list-frame"?

Done. It stretches the content, which is mostly good I think... we don't have a
spec for this, I will ask for one.

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2752223003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:907:
{"networkSectionAdvancedExpandA11yLabel",
On 2017/03/20 19:11:31, dschuyler wrote:
> Would it make sense to s/Expand// here? I'm not sure, I'm just pattern
matching
> vs. the IDS_ on line 908.

Sure. Done.

[stevenjb, 2017-03-27 19:49:19]

ping

[dschuyler, 2017-03-28 01:38:06]

LGTM. The remaining suggestions are intended to be helpful, but the CL can land
without changing them.

https://codereview.chromium.org/2752223003/diff/40001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2752223003/diff/40001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1100: <message
name="IDS_SETTINGS_INTERNET_NETWORK_SECTION_NETWORK_ACCESSIBILITY_LABEL"
desc="Label for the button that toggles showing the network address
configuration settings. Only visible by screen reader software.">
Not critical, just a critique:

IDS_SETTINGS_INTERNET_NETWORK_SECTION_NETWORK_ACCESSIBILITY_LABEL is getting
rather long, and when I tried breaking down what it's trying to say, I didn't
get "Show network address settings". Would something like
IDS_SETTINGS_NETWORK_SHOW_ADDRESS be enough here?

Though I (and others?) mainly use these tags to grep for the i18n tag.  Using a
random set of characters would likely work for that use case. Though I think
(and may be wrong) that translators use these tag names. For that use case I'm
concerned about
IDS_SETTINGS_INTERNET_NETWORK_SECTION_NETWORK_ACCESSIBILITY_LABEL be being
wordy.

https://codereview.chromium.org/2752223003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_detail_page.html
(right):

https://codereview.chromium.org/2752223003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:249:
<div class="layout vertical indented">
Would "settings-box continuation" work here?

https://codereview.chromium.org/2752223003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_proxy.html (right):

https://codereview.chromium.org/2752223003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.html:72: <div
class="settings-box continuation indent"
I'm glad the .list-frame worked out in the cases above. Can it be further
applied here and below?

[stevenjb, 2017-03-29 20:56:10]

https://codereview.chromium.org/2752223003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_detail_page.html
(right):

https://codereview.chromium.org/2752223003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_detail_page.html:249:
<div class="layout vertical indented">
On 2017/03/28 01:38:06, dschuyler wrote:
> Would "settings-box continuation" work here?

No, it would loose the 'vertical' layout. Also, each of the pieces below have
custom layout, they don't really fit the 'settings-box' mold; the only thing
they share in common is the indent amount, which is handled by a CSS var.

https://codereview.chromium.org/2752223003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_proxy.html (right):

https://codereview.chromium.org/2752223003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.html:72: <div
class="settings-box continuation indent"
On 2017/03/28 01:38:06, dschuyler wrote:
> I'm glad the .list-frame worked out in the cases above. Can it be further
> applied here and below?

This one would work, but only because the single control spans the entire width.
The others would break. I'd rather keep all of the "control" boxes as
settings-box.

[stevenjb, 2017-03-29 20:56:13]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2017-03-29 20:56:58]

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

[dschuyler, 2017-03-29 21:54:33]

The CQ bit was unchecked by dschuyler@chromium.org

[dschuyler, 2017-03-29 21:54:53]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2017-03-29 21:56:10]

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

[chromium-reviews, 2017-03-29 21:56:51]

Unchecking the cq was a whoops. I checked it again right after.

On Wed, Mar 29, 2017 at 2:56 PM, commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> CQ is trying da patch. Follow status at
>
> https://chromium-cq-status.appspot.com/v2/patch-status/
> codereview.chromium.org/2752223003/80001
>
>
> https://codereview.chromium.org/2752223003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-03-29 23:14:28]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490824493689440,
"parent_rev": "f5bc337d0967c674ab2457131e558a003f0104c2", "commit_rev":
"fe08f3e8ddd0da65fe91e2537fc29d0acbcc20a4"}

[commit-bot: I haz the power, 2017-03-29 23:15:39]

Description was changed from

==========
MD Settings: Fix Network section styling

This CL makes Settings specific changes to the 'Network' (was 'Internet
conneciton') section.

See issue for specs and screenshots.

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

to

==========
MD Settings: Fix Network section styling

This CL makes Settings specific changes to the 'Network' (was 'Internet
conneciton') section.

See issue for specs and screenshots.

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

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

[commit-bot: I haz the power, 2017-03-29 23:15:41]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/fe08f3e8ddd0da65fe91e2537fc2...