MD Settings: Intenet: Fix network list

MD Settings: Intenet: Fix network list

This CL:
* Fixes animation when using dom-if + iron-collapse.
* Uses CrScrollableBehavior to get keyboard focus and top/botom
  separators.

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

Committed: https://crrev.com/e4c61a66cfa8a058dd7211be4d57fcc57f6c4767
Cr-Commit-Position: refs/heads/master@{#413465}

[stevenjb, 2016-08-18 18:58:35]

Description was changed from

==========
MD Settings: Intenet: Fix network list

This CL:
* Fixes animation when using dom-if + iron-collapse.
* Uses CrScrollableBehavior to get keyboard focus and top/botom
  separators.

BUG=609156
==========

to

==========
MD Settings: Intenet: Fix network list

This CL:
* Fixes animation when using dom-if + iron-collapse.
* Uses CrScrollableBehavior to get keyboard focus and top/botom
  separators.

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

[stevenjb, 2016-08-18 19:27:29]

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

[stevenjb, 2016-08-18 19:27:30]

This was the original motivation for CrScrollableBehavior...

[dschuyler, 2016-08-18 23:30:18]

LGTM with requests.

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

https://codereview.chromium.org/2260613003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:64:
<iron-collapse opened="[[ironCollapseIsOpened_(expanded, wasExpanded)]]">
nit: The function names domIfIsVisible_ and 
ironCollapseIsOpened_ seem like they could
be improved. Maybe something about 
networkList instead of domIf and ironCollapse?

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

https://codereview.chromium.org/2260613003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:110: /**
I think this needs a comment. Is this what this
function is doing?:

* Allow |expanded| to toggle until |wasExpanded| 
* is set. Once |wasExpanded| is true, always return true.

I realize that the above essentially says what the function
is doing (thus wouldn't need a comment). I think this is
involved enough to deserve a comment though. If I'm 
wrong about what it's doing, then it needs a comment even 
more :)

https://codereview.chromium.org/2260613003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:126: /**
Please add a comment. Maybe:

* Always false until the dom-if is expanded.

[stevenjb, 2016-08-19 00:17:13]

Patchset #4 (id:60001) has been deleted

[stevenjb, 2016-08-19 00:19:05]

Patchset #4 (id:80001) has been deleted

[stevenjb, 2016-08-19 00:20:43]

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

https://codereview.chromium.org/2260613003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:64:
<iron-collapse opened="[[ironCollapseIsOpened_(expanded, wasExpanded)]]">
On 2016/08/18 23:30:18, dschuyler wrote:
> nit: The function names domIfIsVisible_ and 
> ironCollapseIsOpened_ seem like they could
> be improved. Maybe something about 
> networkList instead of domIf and ironCollapse?

You mean like networkListDomIfIsVisible? That's getting pretty lengthy, but
sure...

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

https://codereview.chromium.org/2260613003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:110: /**
On 2016/08/18 23:30:18, dschuyler wrote:
> I think this needs a comment. Is this what this
> function is doing?:
> 
> * Allow |expanded| to toggle until |wasExpanded| 
> * is set. Once |wasExpanded| is true, always return true.
> 
> I realize that the above essentially says what the function
> is doing (thus wouldn't need a comment). I think this is
> involved enough to deserve a comment though. If I'm 
> wrong about what it's doing, then it needs a comment even 
> more :)

Added a hopefully helpful comment.

https://codereview.chromium.org/2260613003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.js:126: /**
On 2016/08/18 23:30:18, dschuyler wrote:
> Please add a comment. Maybe:
> 
> * Always false until the dom-if is expanded.

Acknowledged.

[dschuyler, 2016-08-19 21:05:20]

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

https://codereview.chromium.org/2260613003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:64:
<iron-collapse opened="[[ironCollapseIsOpened_(expanded, wasExpanded)]]">
On 2016/08/19 00:20:43, stevenjb wrote:
> On 2016/08/18 23:30:18, dschuyler wrote:
> > nit: The function names domIfIsVisible_ and 
> > ironCollapseIsOpened_ seem like they could
> > be improved. Maybe something about 
> > networkList instead of domIf and ironCollapse?
> 
> You mean like networkListDomIfIsVisible? That's getting pretty lengthy, but
> sure...
> 

This is just a suggestion, feel free to ignore it.
I thought domIfIsVisible was too generic. I agree
with not making it too long. Maybe 
isNetworkListVisible_

That would hold up better if a second dom-if were
added or if that dom-if were changed to hidden or
something else.

Maybe just isVisible_ would be ok too, but I prefer
isNetworkListVislbe_

Thanks for asking, so I could clarify what I meant. 
Whichever you decide on (even domIfIsVisible_, if ya
really feel that is best) is a LGTM.

[stevenjb, 2016-08-22 15:16:02]

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

https://codereview.chromium.org/2260613003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:64:
<iron-collapse opened="[[ironCollapseIsOpened_(expanded, wasExpanded)]]">
On 2016/08/19 21:05:20, dschuyler wrote:
> On 2016/08/19 00:20:43, stevenjb wrote:
> > On 2016/08/18 23:30:18, dschuyler wrote:
> > > nit: The function names domIfIsVisible_ and 
> > > ironCollapseIsOpened_ seem like they could
> > > be improved. Maybe something about 
> > > networkList instead of domIf and ironCollapse?
> > 
> > You mean like networkListDomIfIsVisible? That's getting pretty lengthy, but
> > sure...
> > 
> 
> This is just a suggestion, feel free to ignore it.
> I thought domIfIsVisible was too generic. I agree
> with not making it too long. Maybe 
> isNetworkListVisible_
> 
> That would hold up better if a second dom-if were
> added or if that dom-if were changed to hidden or
> something else.
> 
> Maybe just isVisible_ would be ok too, but I prefer
> isNetworkListVislbe_
> 
> Thanks for asking, so I could clarify what I meant. 
> Whichever you decide on (even domIfIsVisible_, if ya
> really feel that is best) is a LGTM.

I appreciate the suggestion, and after some though came up with
'networksDomIfIsTrue' (since dom-if is truthy, not shown/hidden). Not perfect,
but names are hard :)

[stevenjb, 2016-08-22 15:16:10]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2016-08-22 15:16:38]

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, 2016-08-22 17:25:06]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-08-22 17:28:07]

Description was changed from

==========
MD Settings: Intenet: Fix network list

This CL:
* Fixes animation when using dom-if + iron-collapse.
* Uses CrScrollableBehavior to get keyboard focus and top/botom
  separators.

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

to

==========
MD Settings: Intenet: Fix network list

This CL:
* Fixes animation when using dom-if + iron-collapse.
* Uses CrScrollableBehavior to get keyboard focus and top/botom
  separators.

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

Committed: https://crrev.com/e4c61a66cfa8a058dd7211be4d57fcc57f6c4767
Cr-Commit-Position: refs/heads/master@{#413465}
==========

[commit-bot: I haz the power, 2016-08-22 17:28:08]

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