Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
108163005:
[Files.app] Remove OK and Cancel buttons in task picker dialog (Closed)
|
Created:
7 years ago by yoshiki Modified:
6 years, 11 months ago CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Files.app] Remove OK and Cancel buttons in task picker dialog
BUG=223574
TEST=manually tested
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243526
Patch Set 1 #
Total comments: 8
Patch Set 2 : addressed a comment #
Total comments: 1
Patch Set 3 : reupload #
Total comments: 2
Patch Set 4 : addressed comments #
Total comments: 4
Patch Set 5 : Adressed the comment #
Total comments: 6
Patch Set 6 : addressed comments #Patch Set 7 : rebase #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/108163005/diff/1/chrome/browser/resources/fil... File chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js (right): https://codereview.chromium.org/108163005/diff/1/chrome/browser/resources/fil... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:96: * @param {function(Object=)} onSelected Callback with the selected item. An argument of the callback function seems not to be optional. https://codereview.chromium.org/108163005/diff/1/chrome/browser/resources/fil... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:99: DefaultActionDialog.prototype.show = function(title, message, items, Could you move the file into the js/ui directory? https://codereview.chromium.org/108163005/diff/1/chrome/browser/resources/fil... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:139: * @override This is no longer override method. https://codereview.chromium.org/108163005/diff/1/chrome/browser/resources/fil... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:154: this.onSelected_(); According to the comments of crbug.com/222941, clicking an item should close the dialog and run the application. But the double click or enter key is needed to do in the current implementation.
@hirono, PTAL. Thanks. https://codereview.chromium.org/108163005/diff/1/chrome/browser/resources/fil... File chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js (right): https://codereview.chromium.org/108163005/diff/1/chrome/browser/resources/fil... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:96: * @param {function(Object=)} onSelected Callback with the selected item. On 2013/12/09 06:30:16, hirono wrote: > An argument of the callback function seems not to be optional. Done. https://codereview.chromium.org/108163005/diff/1/chrome/browser/resources/fil... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:99: DefaultActionDialog.prototype.show = function(title, message, items, On 2013/12/09 06:30:16, hirono wrote: > Could you move the file into the js/ui directory? I think so, but there are some other files which should be re-organized. Let's do it in a separate patch. https://codereview.chromium.org/108163005/diff/1/chrome/browser/resources/fil... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:139: * @override On 2013/12/09 06:30:16, hirono wrote: > This is no longer override method. Done. https://codereview.chromium.org/108163005/diff/1/chrome/browser/resources/fil... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:154: this.onSelected_(); On 2013/12/09 06:30:16, hirono wrote: > According to the comments of crbug.com/222941, clicking an item should close the > dialog and run the application. But the double click or enter key is needed to > do in the current implementation. Done.
I tried this patch locally. It seems the open-with menu does not open the task picker. https://codereview.chromium.org/108163005/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js (right): https://codereview.chromium.org/108163005/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:40: t'change', this.onSelected_.bind(this)); nit: remove t.
On 2013/12/13 03:31:09, hirono wrote: > I tried this patch locally. > It seems the open-with menu does not open the task picker. Please apply the following patch as well. https://codereview.chromium.org/109143005/
And I'm sorry that I forgot to upload the latest patch. Could you take a look at new patchset again?
On 2013/12/13 06:11:01, yoshiki wrote: > And I'm sorry that I forgot to upload the latest patch. Could you take a look at > new patchset again? I currently see Patch Set 2. Is it the latest one?
https://codereview.chromium.org/108163005/diff/40001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js (right): https://codereview.chromium.org/108163005/diff/40001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:105: this.onCancelledCallback_ = opt_onCancel || function() {}; Will this called when the close button is clicked or the ESC key is pushed?
@hirono: PTAL. Thanks. https://codereview.chromium.org/108163005/diff/40001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js (right): https://codereview.chromium.org/108163005/diff/40001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:105: this.onCancelledCallback_ = opt_onCancel || function() {}; On 2013/12/13 06:50:10, hirono wrote: > Will this called when the close button is clicked or the ESC key is pushed? Good point! It never been called. Removed this and 6th argument (since it is never used from anywhere).
lgtm! Thanks!
On 2013/12/13 08:02:12, hirono wrote: > lgtm! Thanks! Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/108163005/60001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/108163005/60001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
@arv: could you take a look at dialogs.js? Thanks
Not LGTM https://codereview.chromium.org/108163005/diff/60001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/ui/file_manager_dialog_base.js (right): https://codereview.chromium.org/108163005/diff/60001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/ui/file_manager_dialog_base.js:106: this.buttons_.style.display = 'none'; buttons_ is private and should not be touched. At some point we will start to use real private state and this will break. https://codereview.chromium.org/108163005/diff/60001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/dialogs.js (right): https://codereview.chromium.org/108163005/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/dialogs.js:69: this.buttons_ = doc.createElement('div'); Add accessor or name it this.buttons.
@arv: PTAL again. Thanks. https://codereview.chromium.org/108163005/diff/60001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/ui/file_manager_dialog_base.js (right): https://codereview.chromium.org/108163005/diff/60001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/ui/file_manager_dialog_base.js:106: this.buttons_.style.display = 'none'; On 2013/12/16 16:51:28, arv wrote: > buttons_ is private and should not be touched. At some point we will start to > use real private state and this will break. I got it. Renamed "buttons_" to "button". https://codereview.chromium.org/108163005/diff/60001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/dialogs.js (right): https://codereview.chromium.org/108163005/diff/60001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/dialogs.js:69: this.buttons_ = doc.createElement('div'); On 2013/12/16 16:51:28, arv wrote: > Add accessor or name it this.buttons. Renamed "buttons_" to "button".
LGTM https://codereview.chromium.org/108163005/diff/70001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js (right): https://codereview.chromium.org/108163005/diff/70001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:98: * @param {function(Object)} onSelected Callback which called when an item is Callback which _is_ called ... https://codereview.chromium.org/108163005/diff/70001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:102: defaultIndex, onSelectedItem) { The @param does not match https://codereview.chromium.org/108163005/diff/70001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:152: this.hide(); wrong indentation
Thanks! https://codereview.chromium.org/108163005/diff/70001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js (right): https://codereview.chromium.org/108163005/diff/70001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:98: * @param {function(Object)} onSelected Callback which called when an item is On 2014/01/06 15:13:34, arv wrote: > Callback which _is_ called ... Done. https://codereview.chromium.org/108163005/diff/70001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:102: defaultIndex, onSelectedItem) { On 2014/01/06 15:13:34, arv wrote: > The @param does not match Done. https://codereview.chromium.org/108163005/diff/70001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/default_action_dialog.js:152: this.hide(); On 2014/01/06 15:13:34, arv wrote: > wrong indentation Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/108163005/280001
Retried try job too often on win_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/108163005/280001
Message was sent while issue was closed.
Change committed as 243526 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
