[MD Bookmarks] Make folder nodes extend their whole width.

[MD Bookmarks] Make folder nodes extend their whole width.

This CL makes the folder nodes in the bookmarks sidebar fill the whole
width of the sidebar so that they can be highlighted and rippled on in
the future.

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

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

[calamity, 2017-03-07 06:41:09]

Description was changed from

==========
[MD Bookmarks] Make folder nodes extend their whole width.

This CL makes the folder nodes in the bookmarks sidebar fill the whole
width of the sidebar so that they can be highlighted and rippled on in
the future.

BUG=658980
==========

to

==========
[MD Bookmarks] Make folder nodes extend their whole width.

This CL makes the folder nodes in the bookmarks sidebar fill the whole
width of the sidebar so that they can be highlighted and rippled on in
the future.

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

[calamity, 2017-03-07 07:28:11]

Description was changed from

==========
[MD Bookmarks] Make folder nodes extend their whole width.

This CL makes the folder nodes in the bookmarks sidebar fill the whole
width of the sidebar so that they can be highlighted and rippled on in
the future.

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

to

==========
[MD Bookmarks] Make folder nodes extend their whole width.

This CL makes the folder nodes in the bookmarks sidebar fill the whole
width of the sidebar so that they can be highlighted and rippled on in
the future.

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

[calamity, 2017-03-07 07:28:17]

Patchset #1 (id:1) has been deleted

[calamity, 2017-03-07 07:28:29]

calamity@chromium.org changed reviewers:
+ tsergeant@chromium.org

[tsergeant, 2017-03-07 23:32:50]

Cool, generally lg

https://codereview.chromium.org/2733123003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/folder_node.js (right):

https://codereview.chromium.org/2733123003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.js:107:
this.$['folder-label'].style.paddingLeft =
I wonder if there's any way we can do this with custom property magic. Like,

this.$['--node-depth'] = '3'

and then in css:

padding-left: calc(var(--node-depth) * var(--padding-left-per-depth))

Unfortunately, assigning custom properties into this.style doesn't seem to work.

https://codereview.chromium.org/2733123003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/sidebar_test.js (right):

https://codereview.chromium.org/2733123003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/sidebar_test.js:64: assertEquals(
Testing CSS strings seems weird, even if those strings are hardcoded in  JS to
begin with. What about using getComputedStyle() to get the computed values?

[calamity, 2017-03-08 06:52:46]

padding-left changed to margin-left due to downstream necessity. May as well do
it here. Pixel values are fudged. Will be fixed next patch to match mocks.

https://codereview.chromium.org/2733123003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/folder_node.js (right):

https://codereview.chromium.org/2733123003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.js:107:
this.$['folder-label'].style.paddingLeft =
On 2017/03/07 23:32:50, tsergeant wrote:
> I wonder if there's any way we can do this with custom property magic. Like,
> 
> this.$['--node-depth'] = '3'
> 
> and then in css:
> 
> padding-left: calc(var(--node-depth) * var(--padding-left-per-depth))
> 
> Unfortunately, assigning custom properties into this.style doesn't seem to
work.

Magic installed.

https://codereview.chromium.org/2733123003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/sidebar_test.js (right):

https://codereview.chromium.org/2733123003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/sidebar_test.js:64: assertEquals(
On 2017/03/07 23:32:50, tsergeant wrote:
> Testing CSS strings seems weird, even if those strings are hardcoded in  JS to
> begin with. What about using getComputedStyle() to get the computed values?

That seems more brittle to me. This test is mostly here to make sure that depth
updates as we go lower and is reflected into the CSS via substitution, not to
explicitly enforce the correct pixel paddings.

Either way, now the test does that more directly.

[tsergeant, 2017-03-08 23:26:17]

lgtm

I think that if you base this off of https://codereview.chromium.org/2735953002/
it should work and be closer to landable.

https://codereview.chromium.org/2733123003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/folder_node.html (right):

https://codereview.chromium.org/2733123003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:78:
<bookmarks-folder-node item-id="[[child]]" depth="[[getChildDepth_(depth)]]">
Nit: Wrap here.

https://codereview.chromium.org/2733123003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/folder_node.js (right):

https://codereview.chromium.org/2733123003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.js:107:
this.style.setProperty('--node-depth', this.depth);
aaaaaahhhhhhh

(relatedly: CSS custom properties are still really poorly documented)

https://codereview.chromium.org/2733123003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/sidebar_test.js (right):

https://codereview.chromium.org/2733123003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/sidebar_test.js:64: assertEquals('0',
f.style.getPropertyValue('--node-depth'));
👍

[calamity, 2017-03-09 02:46:02]

Patchset #3 (id:60001) has been deleted

[calamity, 2017-03-09 02:48:54]

https://codereview.chromium.org/2733123003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/folder_node.html (right):

https://codereview.chromium.org/2733123003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:78:
<bookmarks-folder-node item-id="[[child]]" depth="[[getChildDepth_(depth)]]">
On 2017/03/08 23:26:17, tsergeant wrote:
> Nit: Wrap here.

Done.

[calamity, 2017-03-10 05:22:36]

Description was changed from

==========
[MD Bookmarks] Make folder nodes extend their whole width.

This CL makes the folder nodes in the bookmarks sidebar fill the whole
width of the sidebar so that they can be highlighted and rippled on in
the future.

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

to

==========
[MD Bookmarks] Make folder nodes extend their whole width.

This CL makes the folder nodes in the bookmarks sidebar fill the whole
width of the sidebar so that they can be highlighted and rippled on in
the future.

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

[calamity, 2017-03-13 03:37:11]

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

[commit-bot: I haz the power, 2017-03-13 03:37:25]

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-03-13 03:49:20]

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

[commit-bot: I haz the power, 2017-03-13 03:49:21]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[calamity, 2017-03-13 04:55:14]

The CQ bit was checked by calamity@chromium.org

[calamity, 2017-03-13 04:55:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org
Link to the patchset: https://codereview.chromium.org/2733123003/#ps120001
(title: "closure")

[commit-bot: I haz the power, 2017-03-13 04:55:20]

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-03-13 05:57:45]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489380914492300,
"parent_rev": "d9f7bd34b3431faf6ce077fc1d85b2335a722931", "commit_rev":
"dedb4c1fcf7a1f1b9d43c3ca4f5b73c10d04c5a5"}

[commit-bot: I haz the power, 2017-03-13 05:58:24]

Description was changed from

==========
[MD Bookmarks] Make folder nodes extend their whole width.

This CL makes the folder nodes in the bookmarks sidebar fill the whole
width of the sidebar so that they can be highlighted and rippled on in
the future.

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

to

==========
[MD Bookmarks] Make folder nodes extend their whole width.

This CL makes the folder nodes in the bookmarks sidebar fill the whole
width of the sidebar so that they can be highlighted and rippled on in
the future.

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

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

[commit-bot: I haz the power, 2017-03-13 05:58:25]

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