[MD Bookmarks] Add drag and drop indicator.

[MD Bookmarks] Add drag and drop indicator.

This CL adds a drag and drop chip which displays the item icon and title
when dragging.

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

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

[calamity, 2017-06-29 05:06:58]

Description was changed from

==========
[MD Bookmarks] Add drag and drop indicator.

This CL adds a drag and drop chip which displays the item icon and title
when dragging.

BUG=729860
==========

to

==========
[MD Bookmarks] Add drag and drop indicator.

This CL adds a drag and drop chip which displays the item icon and title
when dragging.

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

[calamity, 2017-06-29 08:14:29]

Patchset #1 (id:1) has been deleted

[calamity, 2017-06-29 08:14:56]

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

[tsergeant, 2017-06-30 03:31:41]

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

https://codereview.chromium.org/2962923003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_chip.js:39: var elements =
e.detail.elements;
I think 'elements' is a misleading name, because it gives the impression that
you're sending HTMLElements. 'items' is more in line with what we use elsewhere.

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

https://codereview.chromium.org/2962923003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:387:
document.dispatchEvent(new CustomEvent('bookmark-drag-end'));
I'm not a big fan of this way of tying the two elements together.

The way I see it, DNDManager depends on the dnd-chip for a piece of
functionality. The current design doesn't reflect that -- instead, we fire off
the event that happens to have all the information that the chip needs, and hope
that there's a chip which can read it.

This event-based approach is nice when there are multiple places that might use
the event, or if we want to intercept the event as it is bubbling.

That's not the case here, though, so I think we're better off making the
relationship explicit (by having the DND Manager directly call functions on the
chip) rather than implicit. As an added bonus, Closure will actually do
something in this case.

A suggested approach:

1)

get DNDChip() {
  if (!this.chip_)
    this.chip_ = document.createElement(...)
    document.body.appendChild(...)
  return this.chip_;
}

2) Remove the chip in destroy()

3) Make the chip's show/hide methods public and call them here.

WDYT? I'm open to other ideas.

[calamity, 2017-07-03 06:15:07]

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

[commit-bot: I haz the power, 2017-07-03 06:15:16]

Dry run: CQ is trying da patch.

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

[calamity, 2017-07-03 06:52:34]

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

https://codereview.chromium.org/2962923003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_chip.js:39: var elements =
e.detail.elements;
On 2017/06/30 03:31:40, tsergeant wrote:
> I think 'elements' is a misleading name, because it gives the impression that
> you're sending HTMLElements. 'items' is more in line with what we use
elsewhere.

Done.

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

https://codereview.chromium.org/2962923003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:387:
document.dispatchEvent(new CustomEvent('bookmark-drag-end'));
On 2017/06/30 03:31:40, tsergeant wrote:
> I'm not a big fan of this way of tying the two elements together.
> 
> The way I see it, DNDManager depends on the dnd-chip for a piece of
> functionality. The current design doesn't reflect that -- instead, we fire off
> the event that happens to have all the information that the chip needs, and
hope
> that there's a chip which can read it.
> 
> This event-based approach is nice when there are multiple places that might
use
> the event, or if we want to intercept the event as it is bubbling.
> 
> That's not the case here, though, so I think we're better off making the
> relationship explicit (by having the DND Manager directly call functions on
the
> chip) rather than implicit. As an added bonus, Closure will actually do
> something in this case.
> 
> A suggested approach:
> 
> 1)
> 
> get DNDChip() {
>   if (!this.chip_)
>     this.chip_ = document.createElement(...)
>     document.body.appendChild(...)
>   return this.chip_;
> }
> 
> 2) Remove the chip in destroy()
> 
> 3) Make the chip's show/hide methods public and call them here.
> 
> WDYT? I'm open to other ideas.

Done.

[commit-bot: I haz the power, 2017-07-03 07:28:42]

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

[commit-bot: I haz the power, 2017-07-03 07:28:43]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2017-07-04 03:49:08]

lgtm

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

https://codereview.chromium.org/2962923003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:665: get dndChip() {
Nit: @return annotation

[calamity, 2017-07-04 04:30:50]

Also ended up removing the multi-item string for now since it won't be needed in
the planned multi-item UI. Also, fixed an issue where dragging near the edges of
the page would spawn scrollbars.

PTAL, cheers.

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

https://codereview.chromium.org/2962923003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:665: get dndChip() {
On 2017/07/04 03:49:08, tsergeant wrote:
> Nit: @return annotation

Done.

[tsergeant, 2017-07-04 06:02:00]

Neat, I like the simplification.

still lgtm

https://codereview.chromium.org/2962923003/diff/90001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/dnd_manager.js (right):

https://codereview.chromium.org/2962923003/diff/90001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:182: * between items or
highlights folders which are valid drop targ.
Nit: Seems like you deleted a few charact

[calamity, 2017-07-04 08:39:14]

The CQ bit was checked by calamity@chromium.org

[calamity, 2017-07-04 08:39: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/2962923003/#ps110001
(title: " ")

[commit-bot: I haz the power, 2017-07-04 08:39:25]

CQ is trying da patch.

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

[calamity, 2017-07-04 08:39:39]

https://codereview.chromium.org/2962923003/diff/90001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/dnd_manager.js (right):

https://codereview.chromium.org/2962923003/diff/90001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dnd_manager.js:182: * between items or
highlights folders which are valid drop targ.
On 2017/07/04 06:02:00, tsergeant wrote:
> Nit: Seems like you deleted a few charact

Oop

[commit-bot: I haz the power, 2017-07-04 09:53:10]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1499157554293090,
"parent_rev": "41ace186a3e2cb6001a328ca88c9795ace2f11cd", "commit_rev":
"de8088188edebdcfbc05eb14cd5a238c25ae2fc2"}

[commit-bot: I haz the power, 2017-07-04 09:53:25]

Description was changed from

==========
[MD Bookmarks] Add drag and drop indicator.

This CL adds a drag and drop chip which displays the item icon and title
when dragging.

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

to

==========
[MD Bookmarks] Add drag and drop indicator.

This CL adds a drag and drop chip which displays the item icon and title
when dragging.

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

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

[commit-bot: I haz the power, 2017-07-04 09:53:26]

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