[MD Bookmarks] Add draggable sidebar.

[MD Bookmarks] Add draggable sidebar.

This CL adds a draggable splitter which resizes the sidebar. This CL
also repositions the search bar to stay aligned with the main card and
moves the arrows to the left of the folders in the sidebar to allow for
a reasonable overflow behavior.

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

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

[calamity, 2017-03-10 05:21:52]

Description was changed from

==========
[MD Bookmarks] Add draggable sidebar.

This CL adds a draggable splitter which resizes the sidebar. This CL
also repositions the search bar to stay aligned with the main card and
moves the arrows to the left of the folders in the sidebar to allow for
a reasonable overflow behavior.

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

to

==========
[MD Bookmarks] Add draggable sidebar.

This CL adds a draggable splitter which resizes the sidebar. This CL
also repositions the search bar to stay aligned with the main card and
moves the arrows to the left of the folders in the sidebar to allow for
a reasonable overflow behavior.

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

[calamity, 2017-03-10 05:32:41]

Patchset #1 (id:1) has been deleted

[calamity, 2017-03-10 05:34:07]

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

[calamity, 2017-03-10 05:34:07]

This supports most of what Alan wants except for the max width = longest string
+ 8.

[tsergeant, 2017-03-13 02:46:33]

I can't patch this in locally (merge conflicts, plus a missing file), so I
haven't looked closely at the CSS yet.

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

https://codereview.chromium.org/2745593004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:13: hideSplitter_: Boolean,
Nit: \n, @private

https://codereview.chromium.org/2745593004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:32:
bookmarks.Store.getInstance().addObserver(this);
This line shouldn't be necessary? StoreClient already adds this as an observer

https://codereview.chromium.org/2745593004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:36: * Set up the splitter and set
the initial width from localStorage.
@private

https://codereview.chromium.org/2745593004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:61: updateSplitter_: function() {
This only runs on startup, right?

So if the splitter starts out hidden and the window is resized down, will it
hide? (if not, it probably should)

In the case where it needs to be hidden, is there a flash of unhidden splitter
during startup? (if that does happen, can we made the splitter hidden by
default, then unhide it when necessary?)

https://codereview.chromium.org/2745593004/diff/20001/ui/webui/resources/webu...
File ui/webui/resources/webui_resources.grd (right):

https://codereview.chromium.org/2745593004/diff/20001/ui/webui/resources/webu...
ui/webui/resources/webui_resources.grd:332: file="html/cr/ui/splitter.html"
type="chrome_html" />
Looks like you forgot to add this file to git.

[calamity, 2017-03-13 07:04:50]

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

https://codereview.chromium.org/2745593004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:13: hideSplitter_: Boolean,
On 2017/03/13 02:46:33, tsergeant wrote:
> Nit: \n, @private

Done.

https://codereview.chromium.org/2745593004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:32:
bookmarks.Store.getInstance().addObserver(this);
On 2017/03/13 02:46:33, tsergeant wrote:
> This line shouldn't be necessary? StoreClient already adds this as an observer

Oops. Done.

https://codereview.chromium.org/2745593004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:36: * Set up the splitter and set
the initial width from localStorage.
On 2017/03/13 02:46:33, tsergeant wrote:
> @private

Done.

https://codereview.chromium.org/2745593004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:61: updateSplitter_: function() {
On 2017/03/13 02:46:33, tsergeant wrote:
> This only runs on startup, right?
> 
> So if the splitter starts out hidden and the window is resized down, will it
> hide? (if not, it probably should)
> 
> In the case where it needs to be hidden, is there a flash of unhidden splitter
> during startup? (if that does happen, can we made the splitter hidden by
> default, then unhide it when necessary?)

Removed sneaky hiding behavior as discussed.

https://codereview.chromium.org/2745593004/diff/20001/ui/webui/resources/webu...
File ui/webui/resources/webui_resources.grd (right):

https://codereview.chromium.org/2745593004/diff/20001/ui/webui/resources/webu...
ui/webui/resources/webui_resources.grd:332: file="html/cr/ui/splitter.html"
type="chrome_html" />
On 2017/03/13 02:46:33, tsergeant wrote:
> Looks like you forgot to add this file to git.

Oops. One of those things you assume was always there, find it isn't, make it,
then assume it was always there.

[tsergeant, 2017-03-14 03:38:51]

I'm not too keen on how it looks when you scroll the list overflow right
http://imgur.com/zCktKaJ. Any ideas on whether we can improve that? (It might
look better when the idea to hide the splitter until hover is implemented)

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

https://codereview.chromium.org/2745593004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.html:46: <bookmarks-sidebar
id="sidebar">
This can still live on one line.

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

https://codereview.chromium.org/2745593004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:36: padding-left:
calc(var(--node-depth, 0) * var(--margin-left-per-depth));
Oops, didn't think about this last patch:

All these paddings need fixing for RTL.

https://codereview.chromium.org/2745593004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:61: transform:
rotate(270deg);
You can make this look fancy pretty easily:

transform: rotate(-90deg);
transition: transform 150ms;

https://codereview.chromium.org/2745593004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:70: padding-left: 52px;
Can you put a comment in explaining where the magic number comes from?

https://codereview.chromium.org/2745593004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:74: <div id="container"
class="v-centered" on-tap="selectFolder_"`>
There's a stray backtick at the end of the tag

https://codereview.chromium.org/2745593004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:77:
is-open$="[[!isClosed_]]" on-tap="toggleFolder_">
When you click this button, it will also call selectFolder_(), since the
listeners got rearranged.

Is this intentional? If not, stopPropagation from toggleFolder_()

[calamity, 2017-03-14 07:07:55]

Changed layout as discussed. Also shuffled around some min-width stuff to make
things reasonable. The card can get smaller now than before, but the page starts
overflowing at a more reasonable 550px rather than ~800px width.

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

https://codereview.chromium.org/2745593004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.html:46: <bookmarks-sidebar
id="sidebar">
On 2017/03/14 03:38:51, tsergeant wrote:
> This can still live on one line.

Done.

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

https://codereview.chromium.org/2745593004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:36: padding-left:
calc(var(--node-depth, 0) * var(--margin-left-per-depth));
On 2017/03/14 03:38:51, tsergeant wrote:
> Oops, didn't think about this last patch:
> 
> All these paddings need fixing for RTL.

Done.

https://codereview.chromium.org/2745593004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:61: transform:
rotate(270deg);
On 2017/03/14 03:38:51, tsergeant wrote:
> You can make this look fancy pretty easily:
> 
> transform: rotate(-90deg);
> transition: transform 150ms;

So delightful. Wow.

https://codereview.chromium.org/2745593004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:70: padding-left: 52px;
On 2017/03/14 03:38:51, tsergeant wrote:
> Can you put a comment in explaining where the magic number comes from?

Done.

https://codereview.chromium.org/2745593004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:74: <div id="container"
class="v-centered" on-tap="selectFolder_"`>
On 2017/03/14 03:38:51, tsergeant wrote:
> There's a stray backtick at the end of the tag

Oops.

https://codereview.chromium.org/2745593004/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:77:
is-open$="[[!isClosed_]]" on-tap="toggleFolder_">
On 2017/03/14 03:38:51, tsergeant wrote:
> When you click this button, it will also call selectFolder_(), since the
> listeners got rearranged.
> 
> Is this intentional? If not, stopPropagation from toggleFolder_()

Oh good catch. Done.

[tsergeant, 2017-03-15 00:07:52]

A couple more comments about design:

* This works fine when you've got enough folders to show a vertical scrollbar.
When you don't, though, it's pretty jank: you kind of have to know where the
magic div is. I dunno if this has a good solution right now, we might need to
land it and ask Alan.

* Now that the overflow has changed, the search box is hard to use when the
window is small, since you need to scroll right to hit the button. Maybe file a
bug or add a TODO to fix this?

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

https://codereview.chromium.org/2745593004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:29: text-overflow:
ellipsis;
Ellipsizing doesn't seem to work, but it's pretty important when we might not
show the whole title.

https://codereview.chromium.org/2745593004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:62: transform:
rotate(-90deg);
You'll need to adjust this transform in RTL to +90deg

https://codereview.chromium.org/2745593004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.chromium.org/2745593004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.html:19: padding: 20px 32px 20px
17px; /* Left is smaller due to splitter */
you could do this with a calc if you wanted /shruggie

[calamity, 2017-03-15 00:48:19]

Fixed the small search problem. It now has the same scrolling characteristics as
history wrt the toolbar.

Don't think there's a great answer to the magic div.. Maybe a highlight on
hover. The cursor change also serves as an indicator.

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

https://codereview.chromium.org/2745593004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:29: text-overflow:
ellipsis;
On 2017/03/15 00:07:52, tsergeant wrote:
> Ellipsizing doesn't seem to work, but it's pretty important when we might not
> show the whole title.

Done.

https://codereview.chromium.org/2745593004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.html:62: transform:
rotate(-90deg);
On 2017/03/15 00:07:52, tsergeant wrote:
> You'll need to adjust this transform in RTL to +90deg

T_T

https://codereview.chromium.org/2745593004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/list.html (right):

https://codereview.chromium.org/2745593004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.html:19: padding: 20px 32px 20px
17px; /* Left is smaller due to splitter */
On 2017/03/15 00:07:52, tsergeant wrote:
> you could do this with a calc if you wanted /shruggie

Done. /end-shrug

[tsergeant, 2017-03-15 01:59:43]

lgtm

[calamity, 2017-03-15 02:42:03]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2017-03-15 02:42:30]

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-15 02:45:32]

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

[commit-bot: I haz the power, 2017-03-15 02:45:33]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[calamity, 2017-03-15 05:49:43]

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

[calamity, 2017-03-15 05:49:44]

+dbeam for ui/webui/resources OWNERS.

[Dan Beam, 2017-03-15 18:12:30]

lgtm

[calamity, 2017-03-16 03:51:50]

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

[commit-bot: I haz the power, 2017-03-16 03:52:04]

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-16 03:54:08]

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

[commit-bot: I haz the power, 2017-03-16 03:54:09]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[calamity, 2017-03-16 05:31:31]

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

[commit-bot: I haz the power, 2017-03-16 05:32:01]

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-16 06:04:45]

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

[commit-bot: I haz the power, 2017-03-16 06:04:46]

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

[calamity, 2017-03-17 00:27:15]

The CQ bit was checked by calamity@chromium.org

[calamity, 2017-03-17 00:27:16]

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

[commit-bot: I haz the power, 2017-03-17 00:28: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-03-17 01:31:33]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489710435993900,
"parent_rev": "fb5a41a23dda1f926f1d724650ad63d22bb823ac", "commit_rev":
"d1a323946c7ff485c73662eebc30d5f02a070576"}

[commit-bot: I haz the power, 2017-03-17 01:33:20]

Description was changed from

==========
[MD Bookmarks] Add draggable sidebar.

This CL adds a draggable splitter which resizes the sidebar. This CL
also repositions the search bar to stay aligned with the main card and
moves the arrows to the left of the folders in the sidebar to allow for
a reasonable overflow behavior.

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

to

==========
[MD Bookmarks] Add draggable sidebar.

This CL adds a draggable splitter which resizes the sidebar. This CL
also repositions the search bar to stay aligned with the main card and
moves the arrows to the left of the folders in the sidebar to allow for
a reasonable overflow behavior.

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

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

[commit-bot: I haz the power, 2017-03-17 01:33:21]

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