MD settings: Storage: Add DELETE button for deleting Drive offline files.

MD settings: Storage: Add delete icon button for deleting Drive offline files.

Right-arrow icon is not appropriate for a button to delete offline files,
so it should be replaced with delete button.
Deleting 0B of offline files does not make sense, so we should hide
the delete button when the Drive cache size is 0B or unknown.
In addition, this CL makes each row two-line to show the size as secondary text.

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

Review-Url: https://codereview.chromium.org/2663163002
Cr-Commit-Position: refs/heads/master@{#450222}
Committed: https://chromium.googlesource.com/chromium/src/+/7dfc69252853f9c5410584e5087958006fe89da8

[fukino, 2017-01-31 06:14:25]

Description was changed from

==========
MD settings: Storage: Add DELETE button for deleting Drive offline files.

BUG=678383
==========

to

==========
MD settings: Storage: Add DELETE button for deleting Drive offline files.

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

[fukino, 2017-01-31 06:16:44]

Description was changed from

==========
MD settings: Storage: Add DELETE button for deleting Drive offline files.

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

to

==========
MD settings: Storage: Add DELETE button for deleting Drive offline files.

Right-arrow icon is not appropriate for a button to delete offline files,
so it should be replaced with DELETE button.
Deleting 0B of offline files does not make sense, so we should disable
the DELETE button when the Drive cache size is 0B or unknown.

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

[fukino, 2017-01-31 06:19:25]

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

[fukino, 2017-01-31 06:19:26]

Dan, could you take a look?

[Dan Beam, 2017-02-02 01:13:18]

i may not get to this soon, but I pinged bettes@ about this (because I think the
provided mocks are ugly) but he's at some UX convergence thing or something

I've added my thoughts to the bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=678383#c8

[fukino, 2017-02-02 06:37:06]

On 2017/02/02 01:13:18, Dan Beam (slow) wrote:
> i may not get to this soon, but I pinged bettes@ about this (because I think
the
> provided mocks are ugly) but he's at some UX convergence thing or something
> 
> I've added my thoughts to the bug:
> https://bugs.chromium.org/p/chromium/issues/detail?id=678383#c8

I got it.
Thank you for suggesting the improvement. I'll wait a response on the issue.

[Dan Beam, 2017-02-06 17:29:45]

On 2017/02/02 06:37:06, fukino wrote:
> On 2017/02/02 01:13:18, Dan Beam (slow) wrote:
> > i may not get to this soon, but I pinged bettes@ about this (because I think
> the
> > provided mocks are ugly) but he's at some UX convergence thing or something
> > 
> > I've added my thoughts to the bug:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=678383#c8
> 
> I got it.
> Thank you for suggesting the improvement. I'll wait a response on the issue.

bettes@ / sgabriel@ responded on the bug

[fukino, 2017-02-08 20:19:47]

Description was changed from

==========
MD settings: Storage: Add DELETE button for deleting Drive offline files.

Right-arrow icon is not appropriate for a button to delete offline files,
so it should be replaced with DELETE button.
Deleting 0B of offline files does not make sense, so we should disable
the DELETE button when the Drive cache size is 0B or unknown.

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

to

==========
MD settings: Storage: Add delete icon button for deleting Drive offline files.

Right-arrow icon is not appropriate for a button to delete offline files,
so it should be replaced with delete button.
Deleting 0B of offline files does not make sense, so we should hide
the delete button when the Drive cache size is 0B or unknown.
In addition, this CL makes each row two-line to show the size as secondary text.

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

[fukino, 2017-02-08 20:20:51]

Dan, could you take another look?

https://codereview.chromium.org/2663163002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/storage.html (right):

https://codereview.chromium.org/2663163002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/storage.html:129:
button[is='paper-icon-button-light'].delete {
As the delete icon is used only in Chrome OS, I didn't add this to
shared_style_css.html.

https://codereview.chromium.org/2663163002/diff/60001/ui/webui/resources/imag...
File ui/webui/resources/images/delete.svg (right):

https://codereview.chromium.org/2663163002/diff/60001/ui/webui/resources/imag...
ui/webui/resources/images/delete.svg:1: <svg xmlns="http://www.w3.org/2000/svg"
width="24px" height="24px" viewBox="0 0 24 24" fill="#bdbdbd">
I copied SVG from https://icons.googleplex.com/#icon=ic_delete&search=delete,
and replaced the fill color from google-gray-600(#757575) to #bdbdbd to be
consistent with open_in_new.svg in the same directory.

[fukino, 2017-02-13 19:41:17]

Friendly ping :)

[Dan Beam, 2017-02-13 19:47:55]

https://codereview.chromium.org/2663163002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/storage.html (right):

https://codereview.chromium.org/2663163002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/storage.html:201:
hidden$="[[!driveEnabled_]]" actionable$="[[hasDriveCache_]]" >
maybe worth changing from

hidden$="[[!driveEnabled_]]"

to

<template is="dom-if" if="[[driveEnabled_]]">

https://codereview.chromium.org/2663163002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/storage.html:208: <button
class="delete" is="paper-icon-button-light"></button>
can we just use

<link rel="import" href="chrome://resources/cr_elements/icons.html">

<button is="paper-icon-button-light">
  <iron-icon icon="cr:delete"></iron-icon>
</button>

instead of adding a copy of this?

[fukino, 2017-02-13 19:50:52]

https://codereview.chromium.org/2663163002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/storage.html (right):

https://codereview.chromium.org/2663163002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/storage.html:201:
hidden$="[[!driveEnabled_]]" actionable$="[[hasDriveCache_]]" >
On 2017/02/13 19:47:55, Dan Beam wrote:
> maybe worth changing from
> 
> hidden$="[[!driveEnabled_]]"
> 
> to
> 
> <template is="dom-if" if="[[driveEnabled_]]">

Sorry! It's already templated in ToT. I'll rebase this CL.

[fukino, 2017-02-14 00:58:38]

Could you take another look?

https://codereview.chromium.org/2663163002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/storage.html (right):

https://codereview.chromium.org/2663163002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/storage.html:201:
hidden$="[[!driveEnabled_]]" actionable$="[[hasDriveCache_]]" >
On 2017/02/13 19:50:52, fukino wrote:
> On 2017/02/13 19:47:55, Dan Beam wrote:
> > maybe worth changing from
> > 
> > hidden$="[[!driveEnabled_]]"
> > 
> > to
> > 
> > <template is="dom-if" if="[[driveEnabled_]]">
> 
> Sorry! It's already templated in ToT. I'll rebase this CL.

Rebased. (Done)

https://codereview.chromium.org/2663163002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/storage.html:208: <button
class="delete" is="paper-icon-button-light"></button>
On 2017/02/13 19:47:55, Dan Beam wrote:
> can we just use
> 
> <link rel="import" href="chrome://resources/cr_elements/icons.html">
> 
> <button is="paper-icon-button-light">
>   <iron-icon icon="cr:delete"></iron-icon>
> </button>
> 
> instead of adding a copy of this?

<iron-icon icon="cr:delete"> worked. Thanks!
Done.

[Dan Beam, 2017-02-14 01:11:31]

lgtm but you could consider writing a test for this, we have a bunch in
chrome/test/data/webui/settings

[fukino, 2017-02-14 01:24:54]

On 2017/02/14 01:11:31, Dan Beam wrote:
> lgtm but you could consider writing a test for this, we have a bunch in
> chrome/test/data/webui/settings

Thanks!
Can I add the tests in a follow-up CL?
I'll need some time to learn PolymerTest, but I'd like to land the visual change
before the feature freeze if possible.

[Dan Beam, 2017-02-14 01:26:36]

On 2017/02/14 01:24:54, fukino wrote:
> On 2017/02/14 01:11:31, Dan Beam wrote:
> > lgtm but you could consider writing a test for this, we have a bunch in
> > chrome/test/data/webui/settings
> 
> Thanks!
> Can I add the tests in a follow-up CL?
> I'll need some time to learn PolymerTest, but I'd like to land the visual
change
> before the feature freeze if possible.

sure

[fukino, 2017-02-14 01:32:22]

On 2017/02/14 01:26:36, Dan Beam wrote:
> On 2017/02/14 01:24:54, fukino wrote:
> > On 2017/02/14 01:11:31, Dan Beam wrote:
> > > lgtm but you could consider writing a test for this, we have a bunch in
> > > chrome/test/data/webui/settings
> > 
> > Thanks!
> > Can I add the tests in a follow-up CL?
> > I'll need some time to learn PolymerTest, but I'd like to land the visual
> change
> > before the feature freeze if possible.
> 
> sure

Thanks! I'll be able to add tests sometime next week.

[fukino, 2017-02-14 01:32:41]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2017-02-14 01:33:49]

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-02-14 03:41:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487035961835040,
"parent_rev": "24361e8d6f46883ca707f27ece135406f7628d57", "commit_rev":
"7dfc69252853f9c5410584e5087958006fe89da8"}

[commit-bot: I haz the power, 2017-02-14 03:41:53]

Description was changed from

==========
MD settings: Storage: Add delete icon button for deleting Drive offline files.

Right-arrow icon is not appropriate for a button to delete offline files,
so it should be replaced with delete button.
Deleting 0B of offline files does not make sense, so we should hide
the delete button when the Drive cache size is 0B or unknown.
In addition, this CL makes each row two-line to show the size as secondary text.

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

to

==========
MD settings: Storage: Add delete icon button for deleting Drive offline files.

Right-arrow icon is not appropriate for a button to delete offline files,
so it should be replaced with delete button.
Deleting 0B of offline files does not make sense, so we should hide
the delete button when the Drive cache size is 0B or unknown.
In addition, this CL makes each row two-line to show the size as secondary text.

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

Review-Url: https://codereview.chromium.org/2663163002
Cr-Commit-Position: refs/heads/master@{#450222}
Committed:
https://chromium.googlesource.com/chromium/src/+/7dfc69252853f9c5410584e50879...
==========

[commit-bot: I haz the power, 2017-02-14 03:41:54]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/7dfc69252853f9c5410584e50879...