Quick View: Support Drive and beautify UI for quick view.

Quick View: Support Drive and beautify UI for quick view.

Latest mock: https://folio.googleplex.com/crosfilesapppreviewpane/High%20fidelity%20visuals/Quick%20View/06_20_2016

BUG=613465
TEST=manually.

Committed: https://crrev.com/f5664fea0d208a7a8aebba1b4380dc1ad50a9923
Cr-Commit-Position: refs/heads/master@{#402405}

[oka, 2016-06-23 14:41:14]

nit

[oka, 2016-06-23 14:42:12]

Description was changed from

==========
Support Drive and beautify UI for quick view based on Tien's mock.

BUG=613465
TEST=manually.
==========

to

==========
Quick View: Support Drive and beautify UI for quick view.

Latest mock:
https://folio.googleplex.com/crosfilesapppreviewpane/High%20fidelity%20visual...

BUG=613465
TEST=manually.
==========

[oka, 2016-06-23 14:42:12]

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

[oka, 2016-06-23 14:42:19]

PTAL.

[fukino, 2016-06-23 22:37:34]

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.css
(right):

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:11:
background: rgba(0, 0, 0, 0);
background-color: transparent

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:19: flex:
0;
flex: none

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:30:
margin-left: auto;
margin: 0 auto ?

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:36:
background: rgba(0, 0, 0, 0);
ditto

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:45:
background: rgba(0, 0, 0, 0);
ditto

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:55:
background: rgba(0, 0, 0, 0);
ditto

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:113:
background-image: -webkit-image-set(
"Use shorthand notation when possible." according to
https://www.chromium.org/developers/web-development-style-guide.

In this case, you can use 'background' to specify image, position, and repeat.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:116:
background-position: center center;
background-position: center
should be sufficient.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:125:
background-position: center center;
ditto

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:133:
background-position: center center;
ditto

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:141:
background-position: center center;
ditto

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.html
(right):

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.html:46:
<div>No playback available.</div>
Must be internationalized.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.js
(right):

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.js:10:
isImage: Boolean,
nit: as isImage/isAudio/isVideo(/isUnsupported) are exclusive, having one
property (like, type or mediaType?) instead of these three might be better.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.js:43:
onContentPanelTap_: function(event) {
Please add annotation. (If possible, please add annotations to other functions
in this file.)

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.js:45: if(
target.className.split(' ').includes('close-on-click'))
if (target

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.js:45: if(
target.className.split(' ').includes('close-on-click'))
target.classList.contains('close-on-click')

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/quick_view_controller.js
(right):

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:241: * Load
thumbnails from Drive.
nit: Loads a thumbnail ...

[oka, 2016-06-27 03:22:01]

Address some comments

[oka, 2016-06-27 09:33:01]

nit

[oka, 2016-06-27 09:37:52]

Set base origin/master

[oka, 2016-06-27 09:42:19]

nit

[oka, 2016-06-27 11:15:59]

Move head

[oka, 2016-06-27 11:21:20]

nit

[oka, 2016-06-27 11:28:27]

nit

[oka, 2016-06-27 11:29:38]

PTAL

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.css
(right):

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:11:
background: rgba(0, 0, 0, 0);
On 2016/06/23 22:37:33, fukino wrote:
> background-color: transparent

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:19: flex:
0;
On 2016/06/23 22:37:33, fukino wrote:
> flex: none

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:30:
margin-left: auto;
On 2016/06/23 22:37:33, fukino wrote:
> margin: 0 auto ?

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:36:
background: rgba(0, 0, 0, 0);
On 2016/06/23 22:37:33, fukino wrote:
> ditto

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:45:
background: rgba(0, 0, 0, 0);
On 2016/06/23 22:37:33, fukino wrote:
> ditto

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:55:
background: rgba(0, 0, 0, 0);
On 2016/06/23 22:37:33, fukino wrote:
> ditto

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:113:
background-image: -webkit-image-set(
On 2016/06/23 22:37:33, fukino wrote:
> "Use shorthand notation when possible." according to
> https://www.chromium.org/developers/web-development-style-guide.
> 
> In this case, you can use 'background' to specify image, position, and repeat.

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:116:
background-position: center center;
On 2016/06/23 22:37:33, fukino wrote:
> background-position: center
> should be sufficient.

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:125:
background-position: center center;
On 2016/06/23 22:37:33, fukino wrote:
> ditto

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:133:
background-position: center center;
On 2016/06/23 22:37:33, fukino wrote:
> ditto

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:141:
background-position: center center;
On 2016/06/23 22:37:33, fukino wrote:
> ditto

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.html
(right):

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.html:46:
<div>No playback available.</div>
On 2016/06/23 22:37:33, fukino wrote:
> Must be internationalized.

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.js
(right):

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.js:10:
isImage: Boolean,
On 2016/06/23 22:37:34, fukino wrote:
> nit: as isImage/isAudio/isVideo(/isUnsupported) are exclusive, having one
> property (like, type or mediaType?) instead of these three might be better.

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.js:10:
isImage: Boolean,
On 2016/06/23 22:37:34, fukino wrote:
> nit: as isImage/isAudio/isVideo(/isUnsupported) are exclusive, having one
> property (like, type or mediaType?) instead of these three might be better.

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.js:45: if(
target.className.split(' ').includes('close-on-click'))
On 2016/06/23 22:37:34, fukino wrote:
> target.classList.contains('close-on-click')

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.js:45: if(
target.className.split(' ').includes('close-on-click'))
On 2016/06/23 22:37:34, fukino wrote:
> if (target

Done.

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/quick_view_controller.js
(right):

https://codereview.chromium.org/2093673002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:241: * Load
thumbnails from Drive.
On 2016/06/23 22:37:34, fukino wrote:
> nit: Loads a thumbnail ...

Done.

[oka, 2016-06-27 11:44:11]

nit

[fukino, 2016-06-27 12:03:31]

lgtm

[oka, 2016-06-27 12:49:53]

The CQ bit was checked by oka@chromium.org

[commit-bot: I haz the power, 2016-06-27 12:50:02]

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

[oka, 2016-06-27 12:50:27]

Thanks!

On Mon, Jun 27, 2016 at 9:50 PM commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

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

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[oka, 2016-06-27 13:38:44]

The CQ bit was checked by oka@chromium.org

[oka, 2016-06-27 13:38:44]

The CQ bit was unchecked by oka@chromium.org

[oka, 2016-06-27 13:40:09]

https://codereview.chromium.org/2093673002/diff/200001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.js
(right):

https://codereview.chromium.org/2093673002/diff/200001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_quick_view.js:12:
isUnsupported: Boolean,
Remove.

[oka, 2016-06-28 05:06:52]

The CQ bit was checked by oka@chromium.org

[commit-bot: I haz the power, 2016-06-28 05:07:03]

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, 2016-06-28 05:11:33]

Description was changed from

==========
Quick View: Support Drive and beautify UI for quick view.

Latest mock:
https://folio.googleplex.com/crosfilesapppreviewpane/High%20fidelity%20visual...

BUG=613465
TEST=manually.
==========

to

==========
Quick View: Support Drive and beautify UI for quick view.

Latest mock:
https://folio.googleplex.com/crosfilesapppreviewpane/High%20fidelity%20visual...

BUG=613465
TEST=manually.
==========

[commit-bot: I haz the power, 2016-06-28 05:11:35]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-06-28 05:13:18]

Description was changed from

==========
Quick View: Support Drive and beautify UI for quick view.

Latest mock:
https://folio.googleplex.com/crosfilesapppreviewpane/High%20fidelity%20visual...

BUG=613465
TEST=manually.
==========

to

==========
Quick View: Support Drive and beautify UI for quick view.

Latest mock:
https://folio.googleplex.com/crosfilesapppreviewpane/High%20fidelity%20visual...

BUG=613465
TEST=manually.

Committed: https://crrev.com/f5664fea0d208a7a8aebba1b4380dc1ad50a9923
Cr-Commit-Position: refs/heads/master@{#402405}
==========

[commit-bot: I haz the power, 2016-06-28 05:13:19]

Patchset 11 (id:??) landed as
https://crrev.com/f5664fea0d208a7a8aebba1b4380dc1ad50a9923
Cr-Commit-Position: refs/heads/master@{#402405}