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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 954523002: Hide details panel when ESC is pressed. (Closed)

Created:
5 years, 10 months ago by Steve McKay
Modified:
5 years, 10 months ago
Reviewers:
hirono
CC:
chromium-reviews, tfarina, rginda+watch_chromium.org, mtomasz+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Hide details panel when ESC is pressed. BUG=460990 TEST=None Committed: https://crrev.com/35e044adc0843c859f0517e2e74974604f14fff3 Cr-Commit-Position: refs/heads/master@{#317764}

Patch Set 1 #

Patch Set 2 : Hide details panel when user hits ESC. #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -0 lines) Patch
M ui/file_manager/file_manager/foreground/js/import_controller.js View 1 chunk +14 lines, -0 lines 4 comments Download

Messages

Total messages: 9 (2 generated)
Steve McKay
Hide details panel when user hits ESC.
5 years, 10 months ago (2015-02-23 18:45:13 UTC) #1
Steve McKay
If you're happy w/ the change, please hit the commit checkbox.
5 years, 10 months ago (2015-02-23 18:55:18 UTC) #3
hirono
lgtm with comments. https://codereview.chromium.org/954523002/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/954523002/diff/20001/ui/file_manager/file_manager/foreground/js/import_controller.js#newcode495 ui/file_manager/file_manager/foreground/js/import_controller.js:495: document.addEventListener('keydown', this.onKeyDown_.bind(this)); Is it OK to ...
5 years, 10 months ago (2015-02-24 07:40:45 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/954523002/20001
5 years, 10 months ago (2015-02-24 07:41:42 UTC) #6
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 10 months ago (2015-02-24 08:19:33 UTC) #7
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/35e044adc0843c859f0517e2e74974604f14fff3 Cr-Commit-Position: refs/heads/master@{#317764}
5 years, 10 months ago (2015-02-24 08:20:22 UTC) #8
Steve McKay
5 years, 10 months ago (2015-02-25 21:22:13 UTC) #9
Message was sent while issue was closed. 
Sent https://codereview.chromium.org/957033002/ with followup change.

https://codereview.chromium.org/954523002/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/import_controller.js (right):

https://codereview.chromium.org/954523002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/import_controller.js:495:
document.addEventListener('keydown', this.onKeyDown_.bind(this));
On 2015/02/24 07:40:45, hirono wrote:
> Is it OK to handle the event by document?
> e.g. If we open context menu or dialog, the ESC key closes both the cloud
import
> banner and dialog box. Searching and renaming are also cancelled by ESC key.

I think this is okay because any click outside of the panel will hide the
details panel.

Thoughts? Should we be just listening on the buttons? Then if user tabs away,
then hits escape...panel will still show.

https://codereview.chromium.org/954523002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/import_controller.js:499: * Handle
document scoped key-down events.
On 2015/02/24 07:40:45, hirono wrote:
> nit: Handles

Done.

Powered by Google App Engine
This is Rietveld 408576698