MD WebUI: Pull 'X selected' toolbar overlay from History into shared element

MD WebUI: Pull 'X selected' toolbar overlay from History into shared element

The overlay is designed for use with cr-toolbar, providing an overlay
with the number of items selected, and a 'cancel' and 'delete' button.
This new shared element will be used by MD Bookmarks.

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

Review-Url: https://codereview.chromium.org/2800603002
Cr-Commit-Position: refs/heads/master@{#463530}
Committed: https://chromium.googlesource.com/chromium/src/+/32875f8b059b68f17b1766f3df508c56b22911c4

[tsergeant, 2017-04-05 04:22:35]

Description was changed from

==========
MD WebUI: Pull 'X selected' toolbar overlay from History into shared element

The overlay is designed for use with cr-toolbar, providing an overlay
with the number of items selected, and a 'cancel' and 'delete' button.
This new shared element will be used by MD Bookmarks.

BUG=692827
==========

to

==========
MD WebUI: Pull 'X selected' toolbar overlay from History into shared element

The overlay is designed for use with cr-toolbar, providing an overlay
with the number of items selected, and a 'cancel' and 'delete' button.
This new shared element will be used by MD Bookmarks.

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

[tsergeant, 2017-04-07 04:33:25]

Patchset #2 (id:20001) has been deleted

[tsergeant, 2017-04-07 04:41:46]

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

[tsergeant, 2017-04-07 04:41:46]

PTAL.

This is mostly just pulling the code straight out to a shared element, with a
little simplification of the CSS while we're there.

[tsergeant, 2017-04-07 04:42:35]

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

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

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

[tsergeant, 2017-04-07 05:09:08]

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

[commit-bot: I haz the power, 2017-04-07 05:09:35]

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-04-07 07:32:18]

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

[commit-bot: I haz the power, 2017-04-07 07:32:18]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2017-04-10 03:21:13]

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

[commit-bot: I haz the power, 2017-04-10 03:21:27]

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-04-10 05:14:05]

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

[commit-bot: I haz the power, 2017-04-10 05:14:07]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-04-10 05:35:35]

I'll look at this Monday my time, btw (sorry for lag)

[Dan Beam, 2017-04-11 00:39:01]

https://codereview.chromium.org/2800603002/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_selection_overlay.html
(right):

https://codereview.chromium.org/2800603002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_selection_overlay.html:44:
<div id="overlay-buttons">
nit: many of these IDs have presentational details in them.  I guess that's not
horrible, but it's better to have "delete" or "cancel" than "-button" in there
in case the presentation changes, IMO.

not a huge deal, just a general gripe of mine.

https://codereview.chromium.org/2800603002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_selection_overlay.html:44:
<div id="overlay-buttons">
why do we need #overlay-buttons rather than putting these styles on :host?

https://codereview.chromium.org/2800603002/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_selection_overlay.js
(right):

https://codereview.chromium.org/2800603002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_selection_overlay.js:28:
onDeleteTap_: function() {
nit: just name this deleteSelectedItems() and make public so we're not calling a
private method from tests?

[tsergeant, 2017-04-11 01:16:20]

Thanks for reviewing!

https://codereview.chromium.org/2800603002/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_selection_overlay.html
(right):

https://codereview.chromium.org/2800603002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_selection_overlay.html:44:
<div id="overlay-buttons">
On 2017/04/11 00:39:01, Dan Beam wrote:
> why do we need #overlay-buttons rather than putting these styles on :host?

The extra div is used to ensure we can have the left spacing while still having
the children be centered with a max-width. I don't know of any way to make that
all behave correctly with only one diff. 

> nit: many of these IDs have presentational details in them.  I guess that's
not
> horrible, but it's better to have "delete" or "cancel" than "-button" in there
> in case the presentation changes, IMO.
> 
> not a huge deal, just a general gripe of mine.

Got it. In this case, I most of the IDs, since they aren't really necessary.

https://codereview.chromium.org/2800603002/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_selection_overlay.js
(right):

https://codereview.chromium.org/2800603002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_selection_overlay.js:28:
onDeleteTap_: function() {
On 2017/04/11 00:39:01, Dan Beam wrote:
> nit: just name this deleteSelectedItems() and make public so we're not calling
a
> private method from tests?

Done, although the method being called from tests is in history_toolbar.js

[tsergeant, 2017-04-11 01:17:26]

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

[commit-bot: I haz the power, 2017-04-11 01:18:11]

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-04-11 01:23:58]

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

[commit-bot: I haz the power, 2017-04-11 01:23:59]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2017-04-11 01:24:49]

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

[commit-bot: I haz the power, 2017-04-11 01:25:43]

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-04-11 02:23:12]

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

[commit-bot: I haz the power, 2017-04-11 02:23:13]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-04-11 03:48:48]

lgtm

[tsergeant, 2017-04-11 03:58:59]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2017-04-11 03:59:31]

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-04-11 04:14:37]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491883139201790,
"parent_rev": "7ff10a8c4f7295430af4d3bac32ffb713fb42761", "commit_rev":
"32875f8b059b68f17b1766f3df508c56b22911c4"}

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

Description was changed from

==========
MD WebUI: Pull 'X selected' toolbar overlay from History into shared element

The overlay is designed for use with cr-toolbar, providing an overlay
with the number of items selected, and a 'cancel' and 'delete' button.
This new shared element will be used by MD Bookmarks.

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

to

==========
MD WebUI: Pull 'X selected' toolbar overlay from History into shared element

The overlay is designed for use with cr-toolbar, providing an overlay
with the number of items selected, and a 'cancel' and 'delete' button.
This new shared element will be used by MD Bookmarks.

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

Review-Url: https://codereview.chromium.org/2800603002
Cr-Commit-Position: refs/heads/master@{#463530}
Committed:
https://chromium.googlesource.com/chromium/src/+/32875f8b059b68f17b1766f3df50...
==========

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

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/32875f8b059b68f17b1766f3df50...