Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(95)

Issue 914353004: Update import details panel to comply (mostly) with UX direction. (Closed)

Created:
5 years, 10 months ago by Steve McKay
Modified:
5 years, 10 months ago
Reviewers:
mtomasz, Ben Kwa, fukino
CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, tfarina, rginda+watch_chromium.org, mtomasz+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Update import details panel to comply (mostly) with UX direction. This is a check-point CL. I'll be following up this change with additional fine-tuning of the UI and some additional tests + the ability to open a new drive window when the destination link is clicked. TBR=cpu@chromium.org for .grd changes BUG=420680 TEST=browser_test: FileManagerJsTest.* Committed: https://crrev.com/b49c0e6f44c59f4a32c53ccf1010f623bdcc4f79 Cr-Commit-Position: refs/heads/master@{#316178}

Patch Set 1 #

Total comments: 48

Patch Set 2 : Respond to review comments. #

Total comments: 6

Patch Set 3 : Send along scan when making a UI update while importing. #

Patch Set 4 : Respond to review comments. #

Patch Set 5 : CSS tweaks #

Patch Set 6 : Merge master. #

Total comments: 3

Patch Set 7 : Exclude details panel button from tab index. #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+407 lines, -197 lines) Patch
M chrome/app/chromeos_strings.grdp View 1 2 3 4 5 1 chunk +55 lines, -13 lines 0 comments Download
M chrome/browser/chromeos/extensions/file_manager/private_api_strings.cc View 1 chunk +40 lines, -12 lines 0 comments Download
M ui/file_manager/file_manager/foreground/css/file_manager.css View 1 2 3 4 5 1 chunk +62 lines, -14 lines 0 comments Download
M ui/file_manager/file_manager/foreground/js/import_controller.js View 1 2 3 12 chunks +224 lines, -132 lines 6 comments Download
M ui/file_manager/file_manager/foreground/js/import_controller_unittest.js View 1 2 3 3 chunks +9 lines, -5 lines 0 comments Download
M ui/file_manager/file_manager/main.html View 1 2 3 4 5 6 3 chunks +17 lines, -21 lines 0 comments Download

Messages

Total messages: 36 (13 generated)
Steve McKay
5 years, 10 months ago (2015-02-12 02:22:57 UTC) #2
mtomasz
lgtm with nits, but please get a quick check from Ben before committing https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.grdp File ...
5 years, 10 months ago (2015-02-12 03:22:02 UTC) #4
Steve McKay
Respond to review comments.
5 years, 10 months ago (2015-02-12 23:14:50 UTC) #6
Steve McKay
Dones. https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.grdp File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.grdp#newcode451 chrome/app/chromeos_strings.grdp:451: <message name="IDS_FILE_BROWSER_CLOUD_IMPORT_TITLE" desc="Title of the cloud import feature."> ...
5 years, 10 months ago (2015-02-12 23:15:54 UTC) #7
Ben Kwa
lgtm lgtm with a couple of nits. https://codereview.chromium.org/914353004/diff/20001/ui/file_manager/file_manager/foreground/js/import_controller.js File ui/file_manager/file_manager/foreground/js/import_controller.js (right): https://codereview.chromium.org/914353004/diff/20001/ui/file_manager/file_manager/foreground/js/import_controller.js#newcode178 ui/file_manager/file_manager/foreground/js/import_controller.js:178: // this.environment_.setCurrentDirectory(); ...
5 years, 10 months ago (2015-02-12 23:31:16 UTC) #8
Steve McKay
Send along scan when making a UI update while importing.
5 years, 10 months ago (2015-02-12 23:32:34 UTC) #10
Steve McKay
Respond to review comments.
5 years, 10 months ago (2015-02-12 23:38:58 UTC) #11
Steve McKay
Dones. Thakns! https://codereview.chromium.org/914353004/diff/20001/ui/file_manager/file_manager/foreground/js/import_controller.js File ui/file_manager/file_manager/foreground/js/import_controller.js (right): https://codereview.chromium.org/914353004/diff/20001/ui/file_manager/file_manager/foreground/js/import_controller.js#newcode178 ui/file_manager/file_manager/foreground/js/import_controller.js:178: // this.environment_.setCurrentDirectory(); On 2015/02/12 23:31:15, Ben Kwa ...
5 years, 10 months ago (2015-02-12 23:39:36 UTC) #12
Steve McKay
CSS tweaks
5 years, 10 months ago (2015-02-12 23:42:05 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/914353004/80001
5 years, 10 months ago (2015-02-12 23:43:27 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator/builds/58355) ios_rel_device_ng on tryserver.chromium.mac (JOB_FAILED, ...
5 years, 10 months ago (2015-02-12 23:46:53 UTC) #19
Steve McKay
Merge master.
5 years, 10 months ago (2015-02-13 01:28:32 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/914353004/100001
5 years, 10 months ago (2015-02-13 01:30:05 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/24594)
5 years, 10 months ago (2015-02-13 02:35:31 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/914353004/100001
5 years, 10 months ago (2015-02-13 05:07:04 UTC) #26
fukino
https://codereview.chromium.org/914353004/diff/100001/ui/file_manager/file_manager/main.html File ui/file_manager/file_manager/main.html (right): https://codereview.chromium.org/914353004/diff/100001/ui/file_manager/file_manager/main.html#newcode384 ui/file_manager/file_manager/main.html:384: <paper-button class="import"> tabindex="-1" for this paper-button will fix the ...
5 years, 10 months ago (2015-02-13 05:26:42 UTC) #28
fukino
https://codereview.chromium.org/914353004/diff/100001/ui/file_manager/file_manager/main.html File ui/file_manager/file_manager/main.html (right): https://codereview.chromium.org/914353004/diff/100001/ui/file_manager/file_manager/main.html#newcode350 ui/file_manager/file_manager/main.html:350: hidden='true'> hidden> should be sufficient. https://codereview.chromium.org/914353004/diff/100001/ui/file_manager/file_manager/main.html#newcode358 ui/file_manager/file_manager/main.html:358: hidden='true'> ditto
5 years, 10 months ago (2015-02-13 05:30:49 UTC) #29
Steve McKay
Exclude details panel button from tab index.
5 years, 10 months ago (2015-02-13 06:03:40 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/914353004/120001
5 years, 10 months ago (2015-02-13 06:03:52 UTC) #32
commit-bot: I haz the power
Committed patchset #7 (id:120001)
5 years, 10 months ago (2015-02-13 06:39:01 UTC) #33
commit-bot: I haz the power
Patchset 7 (id:??) landed as https://crrev.com/b49c0e6f44c59f4a32c53ccf1010f623bdcc4f79 Cr-Commit-Position: refs/heads/master@{#316178}
5 years, 10 months ago (2015-02-13 06:39:46 UTC) #34
mtomasz
See: crbug.com/458510 https://codereview.chromium.org/914353004/diff/120001/ui/file_manager/file_manager/foreground/js/import_controller.js File ui/file_manager/file_manager/foreground/js/import_controller.js (right): https://codereview.chromium.org/914353004/diff/120001/ui/file_manager/file_manager/foreground/js/import_controller.js#newcode603 ui/file_manager/file_manager/foreground/js/import_controller.js:603: this.setDetailsVisible(this.detailsPanel_.className === 'hidden'); Hidden attribute should be ...
5 years, 10 months ago (2015-02-13 13:26:25 UTC) #35
Steve McKay
5 years, 10 months ago (2015-02-13 17:31:45 UTC) #36
Message was sent while issue was closed.
https://codereview.chromium.org/914353004/diff/120001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/import_controller.js (right):

https://codereview.chromium.org/914353004/diff/120001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/import_controller.js:603:
this.setDetailsVisible(this.detailsPanel_.className === 'hidden');
On 2015/02/13 13:26:24, mtomasz wrote:
> Hidden attribute should be used instead?

We both mark the element as hidden AND add the 'hidden' class to the element.
The 'hidden' class provides the smooth and sensual visual transition when
showing/hiding.

https://codereview.chromium.org/914353004/diff/120001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/import_controller.js:609:
this.detailsPanel_.className = '';
On 2015/02/13 13:26:24, mtomasz wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/914353004/diff/120001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/import_controller.js:611:
this.detailsPanel_.className = 'hidden';
On 2015/02/13 13:26:24, mtomasz wrote:
> ditto

Acknowledged.

Powered by Google App Engine
This is Rietveld 408576698