Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12857002:
Files.app: Add subfolders in the left nav (Closed)
|
Created:
7 years, 9 months ago by yoshiki Modified:
7 years, 9 months ago Reviewers:
mtomasz CC:
chromium-reviews, rginda+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFiles.app: Add subfolders in the left nav
This replaces the list view of the roots with the tree view of the directory tree.
BUG=170744
TEST=able to open Downloads and Drive, able to open removable device, able to unmount them.
R=mtomasz@chromium.org
NOTRY=True
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188326
Patch Set 1 #
Total comments: 47
Patch Set 2 : addressed comments #
Total comments: 49
Patch Set 3 : addressed comments #
Total comments: 2
Patch Set 4 : addressed comments #Patch Set 5 : rebase #Messages
Total messages: 15 (0 generated)
@mtomasz: could you take a look? thanks,
This is awesome! It doesn't work on Drive for me, though. Is it going to be implemented later or its a bug? https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/css/file_manager.css (right): https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/css/file_manager.css:201: height: 39px; Remove height: 39px; https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/css/file_manager.css:208: margin: 0 0; 1. Override .expand-icon opacity: 0.5 to 1.0 to match specs. 2. Add margin-left to match specs. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/css/file_manager.css:209: top: 11px; Can we avoid this absolute positioning with a magical number? How about centering the same way as volume-icon? https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/css/file_manager.css:216: margin: 0 0; Is this margin: 0 0 necessary here? https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/css/file_manager.css:277: opacity: 0.7; IS this opacity: 0.7 necessary here? https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/file_manager_commands.js (right): https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/file_manager_commands.js:12: * @return {?DirectoryEntry} Found root. AFAIK objects do not need ? in jsdoc. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/file_manager_commands.js:148: event.canExecute = (rootType === RootType.ARCHIVE || Is this change necessary? RootType.* values are never 0/null/undefined, so == should be always sufficient. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/main_scripts.js (right): https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/main_scripts.js:46: //<include src="../../../../../ui/webui/resources/css/tree.css.js"/> Remove tree.css.js? https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/sidebar.js (right): https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2012 -> 2013 https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:67: */ Can we move to Class.prototype.method = function() style instead? We should choose one style. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:124: event.stopPropagation(); 1. Is stopPropagation() necessary here? 2. Is checking if its unmountable again necessary? 3. Umounting seem not to be working on my linux box. Is it ok? https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:142: return this.firstElementChild.querySelector('.label'); Indentation is off? https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:146: * Invoked when the item is being expended. expended -> expanded https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:153: }); Add .bind(this)? UpdateSubDirectories calls resolveDirectory, etc which may change the this context I think. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:155: e.stopPropagation(); Is this necessary? https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:158: /** Please add a short description. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:166: }, .bind(this) missing. Causes js errors. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:177: if (!dirEntry.createReader) { nit: How about instanceof? Sounds cleaner and less tricky. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:216: /** I think we should have description for the classes, even if it is quite obvious. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:223: * Decorate element. nit: I think we should consistently use passive form. Decorate -> decorates. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:229: (/** @type {DirectoryTree} */ el).decorate(directoryModel); I think this virtual cast is unnecessary, since el.__proto__ already changed the class to DirectoryTree. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/main.html (right): https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/main.html:57: <script src="../../../../ui/webui/resources/css/tree.css.js"></script> Remove this tree.css.js? https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/main.html:89: <script src="js/sidebar.js"></script> nit: Shall it be sorted?
@mtomasz: PTAL https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/css/file_manager.css (right): https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/css/file_manager.css:201: height: 39px; On 2013/03/14 02:52:05, mtomasz wrote: > Remove height: 39px; Done. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/css/file_manager.css:208: margin: 0 0; On 2013/03/14 02:52:05, mtomasz wrote: > 1. Override .expand-icon opacity: 0.5 to 1.0 to match specs. I think we should match to default tree view keeping opacity: 0.5. I'll ask josh about it. > 2. Add margin-left to match specs. Instead of adding margin-left, I added both-side margin and change the width of volume-icon to 16px. This looks like as mock. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/css/file_manager.css:209: top: 11px; On 2013/03/14 02:52:05, mtomasz wrote: > Can we avoid this absolute positioning with a magical number? How about > centering the same way as volume-icon? I changed it vertical-align:center, but to fit it to the center line of the label. I'm keeping top:-1px. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/css/file_manager.css:216: margin: 0 0; On 2013/03/14 02:52:05, mtomasz wrote: > Is this margin: 0 0 necessary here? removed. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/css/file_manager.css:277: opacity: 0.7; On 2013/03/14 02:52:05, mtomasz wrote: > IS this opacity: 0.7 necessary here? It exists original on eject icon hence I'm keeping this. It will be changed to 1.0 on hover (see below). https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/file_manager_commands.js (right): https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/file_manager_commands.js:12: * @return {?DirectoryEntry} Found root. On 2013/03/14 02:52:05, mtomasz wrote: > AFAIK objects do not need ? in jsdoc. Oops, removed. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/file_manager_commands.js:148: event.canExecute = (rootType === RootType.ARCHIVE || On 2013/03/14 02:52:05, mtomasz wrote: > Is this change necessary? RootType.* values are never 0/null/undefined, so == > should be always sufficient. Done. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/main_scripts.js (right): https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/main_scripts.js:46: //<include src="../../../../../ui/webui/resources/css/tree.css.js"/> On 2013/03/14 02:52:05, mtomasz wrote: > Remove tree.css.js? We need it to animate an expand triangle on left of tree item. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/sidebar.js (right): https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/03/14 02:52:05, mtomasz wrote: > 2012 -> 2013 Done. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:67: */ On 2013/03/14 02:52:05, mtomasz wrote: > Can we move to Class.prototype.method = function() style instead? We should > choose one style. Done. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:124: event.stopPropagation(); On 2013/03/14 02:52:05, mtomasz wrote: > 1. Is stopPropagation() necessary here? Yes, it prevents selecting the item when user trys to unmount. > 2. Is checking if its unmountable again necessary? I'm not sure but I added it just in case. > 3. Umounting seem not to be working on my linux box. Is it ok? It's working on a real device. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:142: return this.firstElementChild.querySelector('.label'); On 2013/03/14 02:52:05, mtomasz wrote: > Indentation is off? Done. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:146: * Invoked when the item is being expended. On 2013/03/14 02:52:05, mtomasz wrote: > expended -> expanded Done. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:153: }); On 2013/03/14 02:52:05, mtomasz wrote: > Add .bind(this)? UpdateSubDirectories calls resolveDirectory, etc which may > change the this context I think. Done. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:155: e.stopPropagation(); On 2013/03/14 02:52:05, mtomasz wrote: > Is this necessary? It prevents selecting the item when clicking expand mark. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:158: /** On 2013/03/14 02:52:05, mtomasz wrote: > Please add a short description. Done. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:166: }, On 2013/03/14 02:52:05, mtomasz wrote: > .bind(this) missing. Causes js errors. Done. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:177: if (!dirEntry.createReader) { On 2013/03/14 02:52:05, mtomasz wrote: > nit: How about instanceof? Sounds cleaner and less tricky. Done. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:216: /** On 2013/03/14 02:52:05, mtomasz wrote: > I think we should have description for the classes, even if it is quite obvious. Done. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:223: * Decorate element. On 2013/03/14 02:52:05, mtomasz wrote: > nit: I think we should consistently use passive form. Decorate -> decorates. Done. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:229: (/** @type {DirectoryTree} */ el).decorate(directoryModel); This does nothing on JavaScript interpreter. It's just teaching a cast here to human and JSdoc. On 2013/03/14 02:52:05, mtomasz wrote: > I think this virtual cast is unnecessary, since el.__proto__ already changed the > class to DirectoryTree. https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/main.html (right): https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/main.html:57: <script src="../../../../ui/webui/resources/css/tree.css.js"></script> This is necessary to add animations on expanding/collapsing. On 2013/03/14 02:52:05, mtomasz wrote: > Remove this tree.css.js? https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/main.html:89: <script src="js/sidebar.js"></script> On 2013/03/14 02:52:05, mtomasz wrote: > nit: Shall it be sorted? Done.
https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/sidebar.js (right): https://codereview.chromium.org/12857002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/sidebar.js:124: event.stopPropagation(); > > 3. Umounting seem not to be working on my linux box. Is it ok? > > It's working on a real device. I found some bugs on removable device, but it is fixed on Patch Set #2.
Unmounting via icon doesn't do anything, but when I right click and choose Close then I get a message dialog about unmounting. I think this message should also appear on clicking the eject button. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... File chrome/browser/resources/file_manager/css/file_manager.css (right): https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/css/file_manager.css:1: /* Copyright (c) 2013 The Chromium Authors. All rights reserved. I am not sure about it, but one reviewer told me to set 2013 in new files. As for old, I think we should leave 2012. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/css/file_manager.css:207: margin: 0 2px; This icon is very hard to click using the touch screen, at least on my one. How about increasing the touch area, eg. such trick: height: 37px; margin: 0 -5px; padding: 0 20px; I know it's tricky, but helps a lot. I used it already in the butter bar. WDYT? 2. Shall we make cursor: pointer like on the unmount icon? https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/css/file_manager.css:266: div.root-eject { Please add #roots-list or .root-item to (1) be consistent (2) avoid conflicts in the future. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/css/file_manager.css:281: ditto https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/css/file_manager.css:286: div.root-eject[hidden] { Is this necessary? AFAIK *[hidden] is defined by default. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/css/file_manager.css:292: opacity: 0.7; nit: We have opacity: 0.7 here and in #277. Are both necessary? https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/file_manager.js:520: controller.attachDropTarget(this.directoryTree_, true); nit: Please add comment what is that 'true' argument. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/file_manager.js:953: var currentPath = this.directoryTree_.getCurrentPath() || Indentation is off? I think we can align. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/sidebar.js (right): https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:6: * Update sub-elements of {@code parentElement} reading {@code DirectoryEntry} Update -> updates https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:50: * A directory in the tree. One this element reprecents one directory. One this reprecents -> represents One this -> each https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:96: DirectoryItem.prototype.decorate = This indentation looks strange. I think we never do that. How about: (1) DirectoryItem.prototype.decorate = function( parentDirEntry, parentDirItem, directoryModel) { or (2) DirectoryItem.prototype.decorate = function(parentDirEntry, parentDirItem, directoryModel) { ? https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:100: PathUtil.getRootLabel(path) : parentDirEntry.name; Indentation is off. http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... Search "Ternary". https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:103: this.innerHTML = 1. nit: It's suprising that in html5 quoting is not a must. However, for consistency, I think we should do that. 2. I think we never create elements using innerHTML, besides short messages. I believe this structure should be created via javascript. WDYT? https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:104: '<div class=tree-row>' + Indentation off. 6 -> 4 spaces. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:111: this.setAttribute('role', 'treeitem'); nit: Does ChromeVox work fine with the new sidebar? https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:131: if (iconType === RootType.REMOVABLE) == should be sufficient here. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:165: * Remove line here, since it is only 2 line jsdoc. In #152 it is 3 line, and doesn't have this space. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:187: if (!('createReader' in dirEntry)) { instanceof doesn't work here? https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:198: if (results.length === 0) { nit: I think == 0 is sufficient, or even if (!result.length), since it is an array. In general, we should discuss/check how to deal with ==/===, since everywhere in Files.app is different. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:226: * Tree of directories on sidebar. This element is also the root of items, in nit: on sidebar -> on the sidebar. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:235: * Decorates element. nit: Decorates an element. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:266: * Sets a context menu. Context menu is enabled only archive and removable only -> only on https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:298: * Returns the path of the current selected item. nit: current -> currently (or remove).
@mtomasz: PTAL https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... File chrome/browser/resources/file_manager/css/file_manager.css (right): https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/css/file_manager.css:1: /* Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/03/14 10:15:39, mtomasz wrote: > I am not sure about it, but one reviewer told me to set 2013 in new files. As > for old, I think we should leave 2012. I didn't know but I've just heard same thing. Thanks, https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/css/file_manager.css:207: margin: 0 2px; On 2013/03/14 10:15:39, mtomasz wrote: > This icon is very hard to click using the touch screen, at least on my one. > > How about increasing the touch area, eg. such trick: > height: 37px; > margin: 0 -5px; > padding: 0 20px; > > I know it's tricky, but helps a lot. I used it already in the butter bar. WDYT? I enlarged the expand icon to 37x37. It seems to work well on a touch device. > 2. Shall we make cursor: pointer like on the unmount icon? Good idea! But I think it should be done in original tree.css. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/css/file_manager.css:266: div.root-eject { On 2013/03/14 10:15:39, mtomasz wrote: > Please add #roots-list or .root-item to (1) be consistent (2) avoid conflicts in > the future. Done. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/css/file_manager.css:281: On 2013/03/14 10:15:39, mtomasz wrote: > ditto Done. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/css/file_manager.css:286: div.root-eject[hidden] { On 2013/03/14 10:15:39, mtomasz wrote: > Is this necessary? AFAIK *[hidden] is defined by default. I didn't know thanks! https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/css/file_manager.css:292: opacity: 0.7; On 2013/03/14 10:15:39, mtomasz wrote: > nit: We have opacity: 0.7 here and in #277. Are both necessary? I think Here is unnecessary. Removed. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/file_manager.js:520: controller.attachDropTarget(this.directoryTree_, true); On 2013/03/14 10:15:39, mtomasz wrote: > nit: Please add comment what is that 'true' argument. Done. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/file_manager.js:953: var currentPath = this.directoryTree_.getCurrentPath() || On 2013/03/14 10:15:39, mtomasz wrote: > Indentation is off? I think we can align. Done. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/sidebar.js (right): https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:6: * Update sub-elements of {@code parentElement} reading {@code DirectoryEntry} On 2013/03/14 10:15:39, mtomasz wrote: > Update -> updates Done. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:50: * A directory in the tree. One this element reprecents one directory. One this On 2013/03/14 10:15:39, mtomasz wrote: > reprecents -> represents > One this -> each Done. Thanks! https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:96: DirectoryItem.prototype.decorate = On 2013/03/14 10:15:39, mtomasz wrote: > This indentation looks strange. I think we never do that. How about: > > (1) DirectoryItem.prototype.decorate = function( > parentDirEntry, parentDirItem, directoryModel) { > or > (2) DirectoryItem.prototype.decorate = function(parentDirEntry, > parentDirItem, > directoryModel) { > > ? I prefer to (1). But there are some 'decorate =\n function(...) {' style code. I think there are no much difference and any of three is acceptable for me. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:100: PathUtil.getRootLabel(path) : parentDirEntry.name; On 2013/03/14 10:15:39, mtomasz wrote: > Indentation is off. > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... > > Search "Ternary". Thanks! https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:103: this.innerHTML = nit: It's suprising that in html5 quoting is not a must. However, for > consistency, I think we should do that. Done. > 2. I think we never create elements using innerHTML, besides short messages. I > believe this structure should be created via javascript. WDYT? I think this style is easy to understand what dom are generated. Same style is used in tree.js to generate TreeItem. And I think there are no strong reason to use JavaScript to create the element. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:104: '<div class=tree-row>' + On 2013/03/14 10:15:39, mtomasz wrote: > Indentation off. 6 -> 4 spaces. Done. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:111: this.setAttribute('role', 'treeitem'); On 2013/03/14 10:15:39, mtomasz wrote: > nit: Does ChromeVox work fine with the new sidebar? As I tested, it partially works but it's not completed. cr.ui.Tree seems not to support ChromeVox. However, this attribute is added as same way as tree.js. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:131: if (iconType === RootType.REMOVABLE) On 2013/03/14 10:15:39, mtomasz wrote: > == should be sufficient here. Done. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:165: * On 2013/03/14 10:15:39, mtomasz wrote: > Remove line here, since it is only 2 line jsdoc. In #152 it is 3 line, and > doesn't have this space. Done. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:187: if (!('createReader' in dirEntry)) { On 2013/03/14 10:15:39, mtomasz wrote: > instanceof doesn't work here? instanceof doesn't work here, since we can't get the constructor of DirectoryEntry. Instead, we can use 'dirEntry.constructor.name == "DirectoryEntry"' to check if this is an instance of DirectoryEntry or not. But here the only thing we want to know is we have 'createReader' or not, so I think this is enough for us. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:198: if (results.length === 0) { On 2013/03/14 10:15:39, mtomasz wrote: > nit: I think == 0 is sufficient, or even if (!result.length), since it is an > array. Done. > In general, we should discuss/check how to deal with ==/===, since everywhere in > Files.app is different. I think '===' is safer to use, but '==' is also ok if we use it carefully. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:226: * Tree of directories on sidebar. This element is also the root of items, in On 2013/03/14 10:15:39, mtomasz wrote: > nit: on sidebar -> on the sidebar. Done. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:235: * Decorates element. On 2013/03/14 10:15:39, mtomasz wrote: > nit: Decorates an element. Done. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:266: * Sets a context menu. Context menu is enabled only archive and removable On 2013/03/14 10:15:39, mtomasz wrote: > only -> only on Done. Thanks! https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:298: * Returns the path of the current selected item. On 2013/03/14 10:15:39, mtomasz wrote: > nit: current -> currently (or remove). Done.
How about that unmounting? Can you check? Clicking on the icon doesn't work, but the context menu works. Besides this issue, lgtm with some nits. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... File chrome/browser/resources/file_manager/css/file_manager.css (right): https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/css/file_manager.css:286: div.root-eject[hidden] { On 2013/03/14 13:43:42, yoshiki wrote: > On 2013/03/14 10:15:39, mtomasz wrote: > > Is this necessary? AFAIK *[hidden] is defined by default. > > I didn't know thanks! Sorry, I was wrong. I mean *[hidden] is defined, but it is later overriden in .tree-item > .tree-row > * to display: inline-block, so yet, this is actually needed. https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/sidebar.js (right): https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:187: if (!('createReader' in dirEntry)) { On 2013/03/14 13:43:42, yoshiki wrote: > On 2013/03/14 10:15:39, mtomasz wrote: > > instanceof doesn't work here? > > instanceof doesn't work here, since we can't get the constructor of > DirectoryEntry. Instead, we can use 'dirEntry.constructor.name == > "DirectoryEntry"' to check if this is an instance of DirectoryEntry or not. > > But here the only thing we want to know is we have 'createReader' or not, so I > think this is enough for us. Got it. https://codereview.chromium.org/12857002/diff/29001/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/sidebar.js (right): https://codereview.chromium.org/12857002/diff/29001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:164: * Retrieve the latest subdirectories and update them on the tree. Retrieve -> retrieves and in other places, please check. If the first word is a verb, then it should be in a passive form.
On 2013/03/15 04:23:20, mtomasz wrote: > How about that unmounting? Can you check? Clicking on the icon doesn't work, but > the context menu works. > > Besides this issue, lgtm with some nits. > > https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... > File chrome/browser/resources/file_manager/css/file_manager.css (right): > > https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... > chrome/browser/resources/file_manager/css/file_manager.css:286: > div.root-eject[hidden] { > On 2013/03/14 13:43:42, yoshiki wrote: > > On 2013/03/14 10:15:39, mtomasz wrote: > > > Is this necessary? AFAIK *[hidden] is defined by default. > > > > I didn't know thanks! > > Sorry, I was wrong. I mean *[hidden] is defined, but it is later overriden in > .tree-item > .tree-row > * to display: inline-block, so yet, this is actually > needed. > > https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... > File chrome/browser/resources/file_manager/js/sidebar.js (right): > > https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... > chrome/browser/resources/file_manager/js/sidebar.js:187: if (!('createReader' in > dirEntry)) { > On 2013/03/14 13:43:42, yoshiki wrote: > > On 2013/03/14 10:15:39, mtomasz wrote: > > > instanceof doesn't work here? > > > > instanceof doesn't work here, since we can't get the constructor of > > DirectoryEntry. Instead, we can use 'dirEntry.constructor.name == > > "DirectoryEntry"' to check if this is an instance of DirectoryEntry or not. > > > > But here the only thing we want to know is we have 'createReader' or not, so I > > think this is enough for us. > > Got it. > > https://codereview.chromium.org/12857002/diff/29001/chrome/browser/resources/... > File chrome/browser/resources/file_manager/js/sidebar.js (right): > > https://codereview.chromium.org/12857002/diff/29001/chrome/browser/resources/... > chrome/browser/resources/file_manager/js/sidebar.js:164: * Retrieve the latest > subdirectories and update them on the tree. > Retrieve -> retrieves and in other places, please check. If the first word is a > verb, then it should be in a passive form. One more thing. I think we should listen to metadata changes and update the left nav when directories changes. As for now, if I remove a directory or change it's name, then the left nav gets out of sync. Do you want to do that in a separate cl? This should be 5 lines of code.
On 2013/03/15 04:26:24, mtomasz wrote: > On 2013/03/15 04:23:20, mtomasz wrote: > > How about that unmounting? Can you check? Clicking on the icon doesn't work, > but > > the context menu works. > > > > Besides this issue, lgtm with some nits. > > > > > https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... > > File chrome/browser/resources/file_manager/css/file_manager.css (right): > > > > > https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... > > chrome/browser/resources/file_manager/css/file_manager.css:286: > > div.root-eject[hidden] { > > On 2013/03/14 13:43:42, yoshiki wrote: > > > On 2013/03/14 10:15:39, mtomasz wrote: > > > > Is this necessary? AFAIK *[hidden] is defined by default. > > > > > > I didn't know thanks! > > > > Sorry, I was wrong. I mean *[hidden] is defined, but it is later overriden in > > .tree-item > .tree-row > * to display: inline-block, so yet, this is actually > > needed. > > > > > https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... > > File chrome/browser/resources/file_manager/js/sidebar.js (right): > > > > > https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... > > chrome/browser/resources/file_manager/js/sidebar.js:187: if (!('createReader' > in > > dirEntry)) { > > On 2013/03/14 13:43:42, yoshiki wrote: > > > On 2013/03/14 10:15:39, mtomasz wrote: > > > > instanceof doesn't work here? > > > > > > instanceof doesn't work here, since we can't get the constructor of > > > DirectoryEntry. Instead, we can use 'dirEntry.constructor.name == > > > "DirectoryEntry"' to check if this is an instance of DirectoryEntry or not. > > > > > > But here the only thing we want to know is we have 'createReader' or not, so > I > > > think this is enough for us. > > > > Got it. > > > > > https://codereview.chromium.org/12857002/diff/29001/chrome/browser/resources/... > > File chrome/browser/resources/file_manager/js/sidebar.js (right): > > > > > https://codereview.chromium.org/12857002/diff/29001/chrome/browser/resources/... > > chrome/browser/resources/file_manager/js/sidebar.js:164: * Retrieve the latest > > subdirectories and update them on the tree. > > Retrieve -> retrieves and in other places, please check. If the first word is > a > > verb, then it should be in a passive form. > > One more thing. I think we should listen to metadata changes and update the left > nav when directories changes. As for now, if I remove a directory or change it's > name, then the left nav gets out of sync. Do you want to do that in a separate > cl? This should be 5 lines of code. And also, one more. The selection in the side bar is not synchronized with the file list. If I change directory there, then the selection in the side bar doesn't get updated. It is ok if you want to do that in a separate cl, but please file a bug and write a comment. The same for the previous thing. Thanks.
Thank you for review! > How about that unmounting? Can you check? Clicking on the icon doesn't work, but > the context menu works. I can unmount on a device. The unmount icon doesn't use 'ummount' command and calls unmount() directly. Maybe this is why the unmount icon can't work on mock. I'll create the patch to use the command if possible. > One more thing. I think we should listen to metadata changes and update the left > nav when directories changes. As for now, if I remove a directory or change it's > name, then the left nav gets out of sync. Do you want to do that in a separate > cl? This should be 5 lines of code. Nice catch, I'll do it in a separate patch. > And also, one more. The selection in the side bar is not synchronized with the > file list. If I change directory there, then the selection in the side bar > doesn't get updated. It is ok if you want to do that in a separate cl, but > please file a bug and write a comment. The same for the previous thing. Thanks. I'll do this in a separate patch as well. Thanks, https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... File chrome/browser/resources/file_manager/css/file_manager.css (right): https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... chrome/browser/resources/file_manager/css/file_manager.css:286: div.root-eject[hidden] { On 2013/03/15 04:23:20, mtomasz wrote: > On 2013/03/14 13:43:42, yoshiki wrote: > > On 2013/03/14 10:15:39, mtomasz wrote: > > > Is this necessary? AFAIK *[hidden] is defined by default. > > > > I didn't know thanks! > > Sorry, I was wrong. I mean *[hidden] is defined, but it is later overriden in > .tree-item > .tree-row > * to display: inline-block, so yet, this is actually > needed. Done. https://codereview.chromium.org/12857002/diff/29001/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/sidebar.js (right): https://codereview.chromium.org/12857002/diff/29001/chrome/browser/resources/... chrome/browser/resources/file_manager/js/sidebar.js:164: * Retrieve the latest subdirectories and update them on the tree. On 2013/03/15 04:23:20, mtomasz wrote: > Retrieve -> retrieves and in other places, please check. If the first word is a > verb, then it should be in a passive form. Done.
On 2013/03/15 08:01:06, yoshiki wrote: > Thank you for review! > > > How about that unmounting? Can you check? Clicking on the icon doesn't work, > but > > the context menu works. > > I can unmount on a device. The unmount icon doesn't use > 'ummount' command and calls unmount() directly. Maybe this is why the unmount > icon can't work on mock. I'll create the patch to use the command if possible. > > > One more thing. I think we should listen to metadata changes and update the > left > > nav when directories changes. As for now, if I remove a directory or change > it's > > name, then the left nav gets out of sync. Do you want to do that in a separate > > cl? This should be 5 lines of code. > > Nice catch, I'll do it in a separate patch. > > > And also, one more. The selection in the side bar is not synchronized with the > > file list. If I change directory there, then the selection in the side bar > > doesn't get updated. It is ok if you want to do that in a separate cl, but > > please file a bug and write a comment. The same for the previous thing. > Thanks. > > I'll do this in a separate patch as well. Thanks, > > https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... > File chrome/browser/resources/file_manager/css/file_manager.css (right): > > https://codereview.chromium.org/12857002/diff/13002/chrome/browser/resources/... > chrome/browser/resources/file_manager/css/file_manager.css:286: > div.root-eject[hidden] { > On 2013/03/15 04:23:20, mtomasz wrote: > > On 2013/03/14 13:43:42, yoshiki wrote: > > > On 2013/03/14 10:15:39, mtomasz wrote: > > > > Is this necessary? AFAIK *[hidden] is defined by default. > > > > > > I didn't know thanks! > > > > Sorry, I was wrong. I mean *[hidden] is defined, but it is later overriden in > > .tree-item > .tree-row > * to display: inline-block, so yet, this is actually > > needed. > > Done. > > https://codereview.chromium.org/12857002/diff/29001/chrome/browser/resources/... > File chrome/browser/resources/file_manager/js/sidebar.js (right): > > https://codereview.chromium.org/12857002/diff/29001/chrome/browser/resources/... > chrome/browser/resources/file_manager/js/sidebar.js:164: * Retrieve the latest > subdirectories and update them on the tree. > On 2013/03/15 04:23:20, mtomasz wrote: > > Retrieve -> retrieves and in other places, please check. If the first word is > a > > verb, then it should be in a passive form. > > Done. lgtm. It would be great if the icon uses command, too.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/12857002/39001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/12857002/39001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/12857002/48003
Message was sent while issue was closed.
Change committed as 188326 |
