Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6297013:
[NTP] Allow reordering of apps via drag and drop. (Closed)
|
Created:
9 years, 11 months ago by jstritar Modified:
9 years, 6 months ago CC:
chromium-reviews, Erik does not do reviews, pam+watch_chromium.org, Aaron Boodman, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[NTP] Allow reordering of apps via drag and drop.
BUG=53977
TEST=None.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72311
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 60
Patch Set 3 : incorporate feedback #
Total comments: 2
Patch Set 4 : nits #Patch Set 5 : nit #Messages
Total messages: 10 (0 generated)
Here's an initial take at app drag and drop. I still have some cleanup to do, especially related to the drag and drop delegate interface that the 'apps' object now implements. I'm planning on doing that Monday or Sunday, depending on what time the branch is. There are a couple issues... the first 2 things might be HTML5 drag and drop issues. 1. A default drag image appears next to the cursor that I'm not sure how to hide. The image isn't too bad on mac and windows, but it's pretty annoying on linux. This might be an HTML5 thing.... 2. On Windows, if you drop it out of range, the icon transitions in from the top left rather than where you dropped it. 3. After the branch date, I'm planning on updating the most visited to use this drag and drop code too. 4. When the apps promo is active, there's ~20px extra margin to the left of the web store icon.
On 2011/01/22 00:24:12, jstritar1 wrote: > Here's an initial take at app drag and drop. I still have some cleanup to do, > especially related to the drag and drop delegate interface that the 'apps' > object now implements. I'm planning on doing that Monday or Sunday, depending on > what time the branch is. > > There are a couple issues... the first 2 things might be HTML5 drag and drop > issues. > 1. A default drag image appears next to the cursor that I'm not sure how to > hide. The image isn't too bad on mac and windows, but it's pretty annoying on > linux. This might be an HTML5 thing.... You can set the drag image using dataTransfer.setDragImage. This was not implemented when I wrote the new tab page but it should work now. > 2. On Windows, if you drop it out of range, the icon transitions in from the > top left rather than where you dropped it. This is a regression but that is not due to your refactoring. It happens on the dev channel already. > 3. After the branch date, I'm planning on updating the most visited to use this > drag and drop code too. > 4. When the apps promo is active, there's ~20px extra margin to the left of the > web store icon.
On 2011/01/22 00:24:12, jstritar1 wrote: > 4. When the apps promo is active, there's ~20px extra margin to the left of the > web store icon. That is by design and already happened w/o your change. Did it look like a bug to you?
http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/new... File chrome/browser/resources/new_new_tab.html (right): http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/new... chrome/browser/resources/new_new_tab.html:289: <script src="ntp/draganddrop.js"></script> use underscore as word separator in file names http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp/apps.js (right): http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:295: var ids = []; this.data_ = data.map(function(app) { return app.id; }); http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:404: this.dragContainer.setAttribute('launcher-animations', true); How about introducing a variable to dragContainer. Then you do not need to bind. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:456: if (rtl) left = availableWidth - left - w; line break?
Good work. Lots of comments here, but mostly minor. I tried it out, and it looks and works nicely. The only glitch I noticed is that if you drag an item to the right of the web store entry in detached mode, it registers as a valid drop target and you get placehoder reordering. That doesn't seem right. I believe the reason for this is that you're considering that the grid fills the entire container width, but in detached mode it doesn't. Fixing this might fit nicely into my suggestion to only pass coordinates to the DragContainer's delegate -- that way knowledge about the funky layout of the apps launcher is isolated in that one place. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/dom_ui/app_la... File chrome/browser/dom_ui/app_launcher_handler.cc (right): http://codereview.chromium.org/6297013/diff/2001/chrome/browser/dom_ui/app_la... chrome/browser/dom_ui/app_launcher_handler.cc:173: case NotificationType::EXTENSION_LAUNCHER_REORDERED: Are you sure you need this? I believe that changing the order in ExtensionPrefs will also cause the PREF_CHANGED notification below to fire. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_service.cc (right): http://codereview.chromium.org/6297013/diff/2001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_service.cc:1789: void ExtensionService::SetAppLauncherOrder( This method seems like it makes more sense directly in ExtensionPrefs to me. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/new... File chrome/browser/resources/new_new_tab.js (right): http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/new... chrome/browser/resources/new_new_tab.js:620: apps.visible = true; The visible flag here (and in mostVisited ftm) seems to serve the same purpose that the css classes are. If you rebase, I believe you can just check !classList.contains('disabled') && !classList.contains('hidden') without having the extra flag. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp/apps.css (right): http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.css:79: .app:not(.dragging) > .app-settings:hover { If possible, it would be better to have the .dragging class override the .app class. This way when people add additional rules including .app, they won't have to remember to do :not(.dragging). So for example, if you don't want .app.dragging to have a background image, then have it declare background-image:none. We already have a similar problem with .section:not(.collapsed), but we shouldn't replicate it. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.css:95: -webkit-transition: top .20s, left .20s, right .20s; It seems that the convention in the rest of the css is .2s (as opposed to .20s). http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.css:121: .app.web-store-entry > a { Why this change? http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp/apps.js (right): http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:331: // to be in sync with the rules in apps.css. Is it possible to get them via offsetHeight/offsetWidth instead? Seems like the margins could be gottten via getComputedStyle(). http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:374: saveDrag: function() { Method isn't called from dragdrop.js ? http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:387: layout: function(disable_animations) { It sucks that it is different, but naming in JavaScript is disableAnimations for locals. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:387: layout: function(disable_animations) { Style nit: I don't much care for the boolean flags to methods. You could do: layoutWithAnimations() layoutWithoutAnimations() Or: layout({disableAnimation:true}); http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:410: applyAppRects: function() { Kind of an odd name for this method. What about layoutImpl_? The Impl suffix is a common convention for a method that does the real work of another public facing method. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:419: var t = appsContent.querySelector('[app-id='+apps[i]+']').parentNode; Avoid single variable letter names unless they are extremely obvious (eg: i, j, x, y, w, h), or have well-known mathematical meanings (eg: a^2 + b^2 = c^2). http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:429: t.style.right = ''; It looks like maybe you were going to use 'right', but then decided not to. It is never set to anything. You can remove this line, and another one in dragdropcontroller.js. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:430: t.style.display = this.visible ? '' : 'none'; If this.visible is false, does it mean that the section is just collapsed, or that it is completely hidden. If completely hidden (eg in menu mode), it seems like doing this layout is not even necessary. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:433: innerStyle.left = innerStyle.top = ''; This seems odd, when are these ever used? http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:443: var d = this.dimensions; I don't think it's obvious what d stands for. How about just typing out 'dimensions'. Or if you make the change I suggested above, you could cache a dimensions object in the apps object once it is calculated once and just use that. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp/draganddrop.js (right): http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. Seems like this file should be called drag_drop_controller.js. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:18: // itemHeight, itemWidth, marginWidth, marginHeight, borderWidth Why does this class care about marginWidth/height borderWidth/height? Why not just the overall dimensions? http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:36: this.delegate = delegate; this.delegate_ ? http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:60: return !!this.delegate.dragItem; Nit: prefer Boolean(this.delegate.dragItem) or this.delegate.dragItem != null for clarity. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:63: getIndexAt_: function(e) { Design nit: I think that knowledge of layout should be centralized in the delegate. By having getIndexAt_ in this class, you are baking in the assumption that each drag position can be mapped to an index, and that the index is all the information that the delegate will need in order to decide what to do in canDropOn(). I think it would be more elegant to put getIndexAt_() in apps, and just send the coordinate in canDropOn(). Leave it up to the delegate to decide what to do with it. I might be missing something that makes this complicated though. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:64: var del = this.delegate; Careful with your abbreviations again. Since delegate is only used twice in this class, this.delegate_ both times is fine IMO. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:65: var el = del.dragContainer; This variable is only used once, might as well just have it inline at that time. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:91: if (item) { Invert check and return early. Multiple places in this file. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:145: del.invalidate(); Seems like these two methods should be del.saveDrag(). http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:181: dragItem.style.right = 'auto'; Don't think this line is needed.
Thanks for the feedback. I'm driving into NYC and will finish things up this afternoon. --Jon On Jan 22, 2011, at 6:43 PM, "aa@chromium.org" <aa@chromium.org> wrote: > Good work. Lots of comments here, but mostly minor. > > I tried it out, and it looks and works nicely. The only glitch I noticed is that > if you drag an item to the right of the web store entry in detached mode, it > registers as a valid drop target and you get placehoder reordering. That doesn't > seem right. > > I believe the reason for this is that you're considering that the grid fills the > entire container width, but in detached mode it doesn't. Fixing this might fit > nicely into my suggestion to only pass coordinates to the DragContainer's > delegate -- that way knowledge about the funky layout of the apps launcher is > isolated in that one place. > > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/dom_ui/app_la... > File chrome/browser/dom_ui/app_launcher_handler.cc (right): > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/dom_ui/app_la... > chrome/browser/dom_ui/app_launcher_handler.cc:173: case > NotificationType::EXTENSION_LAUNCHER_REORDERED: > Are you sure you need this? I believe that changing the order in > ExtensionPrefs will also cause the PREF_CHANGED notification below to > fire. > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/extensions/ex... > File chrome/browser/extensions/extension_service.cc (right): > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/extensions/ex... > chrome/browser/extensions/extension_service.cc:1789: void > ExtensionService::SetAppLauncherOrder( > This method seems like it makes more sense directly in ExtensionPrefs to > me. > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/new... > File chrome/browser/resources/new_new_tab.js (right): > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/new... > chrome/browser/resources/new_new_tab.js:620: apps.visible = true; > The visible flag here (and in mostVisited ftm) seems to serve the same > purpose that the css classes are. > > If you rebase, I believe you can just check > !classList.contains('disabled') && !classList.contains('hidden') without > having the extra flag. > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > File chrome/browser/resources/ntp/apps.css (right): > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/apps.css:79: .app:not(.dragging) > > .app-settings:hover { > If possible, it would be better to have the .dragging class override the > .app class. This way when people add additional rules including .app, > they won't have to remember to do :not(.dragging). > > So for example, if you don't want .app.dragging to have a background > image, then have it declare background-image:none. > > We already have a similar problem with .section:not(.collapsed), but we > shouldn't replicate it. > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/apps.css:95: -webkit-transition: top .20s, > left .20s, right .20s; > It seems that the convention in the rest of the css is .2s (as opposed > to .20s). > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/apps.css:121: .app.web-store-entry > a { > Why this change? > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > File chrome/browser/resources/ntp/apps.js (right): > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/apps.js:331: // to be in sync with the > rules in apps.css. > Is it possible to get them via offsetHeight/offsetWidth instead? Seems > like the margins could be gottten via getComputedStyle(). > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/apps.js:374: saveDrag: function() { > Method isn't called from dragdrop.js ? > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/apps.js:387: layout: > function(disable_animations) { > It sucks that it is different, but naming in JavaScript is > disableAnimations for locals. > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/apps.js:387: layout: > function(disable_animations) { > Style nit: I don't much care for the boolean flags to methods. > > You could do: > > layoutWithAnimations() > layoutWithoutAnimations() > > Or: > > layout({disableAnimation:true}); > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/apps.js:410: applyAppRects: function() { > Kind of an odd name for this method. What about layoutImpl_? The Impl > suffix is a common convention for a method that does the real work of > another public facing method. > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/apps.js:419: var t = > appsContent.querySelector('[app-id='+apps[i]+']').parentNode; > Avoid single variable letter names unless they are extremely obvious > (eg: i, j, x, y, w, h), or have well-known mathematical meanings (eg: > a^2 + b^2 = c^2). > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/apps.js:429: t.style.right = ''; > It looks like maybe you were going to use 'right', but then decided not > to. It is never set to anything. You can remove this line, and another > one in dragdropcontroller.js. > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/apps.js:430: t.style.display = this.visible > ? '' : 'none'; > If this.visible is false, does it mean that the section is just > collapsed, or that it is completely hidden. > > If completely hidden (eg in menu mode), it seems like doing this layout > is not even necessary. > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/apps.js:433: innerStyle.left = > innerStyle.top = ''; > This seems odd, when are these ever used? > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/apps.js:443: var d = this.dimensions; > I don't think it's obvious what d stands for. How about just typing out > 'dimensions'. Or if you make the change I suggested above, you could > cache a dimensions object in the apps object once it is calculated once > and just use that. > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > File chrome/browser/resources/ntp/draganddrop.js (right): > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/draganddrop.js:1: // Copyright (c) 2011 The > Chromium Authors. All rights reserved. > Seems like this file should be called drag_drop_controller.js. > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/draganddrop.js:18: // itemHeight, > itemWidth, marginWidth, marginHeight, borderWidth > Why does this class care about marginWidth/height borderWidth/height? > Why not just the overall dimensions? > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/draganddrop.js:36: this.delegate = > delegate; > this.delegate_ ? > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/draganddrop.js:60: return > !!this.delegate.dragItem; > Nit: prefer Boolean(this.delegate.dragItem) or this.delegate.dragItem != > null for clarity. > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/draganddrop.js:63: getIndexAt_: function(e) > { > Design nit: I think that knowledge of layout should be centralized in > the delegate. By having getIndexAt_ in this class, you are baking in the > assumption that each drag position can be mapped to an index, and that > the index is all the information that the delegate will need in order to > decide what to do in canDropOn(). > > I think it would be more elegant to put getIndexAt_() in apps, and just > send the coordinate in canDropOn(). Leave it up to the delegate to > decide what to do with it. > > I might be missing something that makes this complicated though. > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/draganddrop.js:64: var del = this.delegate; > Careful with your abbreviations again. Since delegate is only used twice > in this class, this.delegate_ both times is fine IMO. > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/draganddrop.js:65: var el = > del.dragContainer; > This variable is only used once, might as well just have it inline at > that time. > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/draganddrop.js:91: if (item) { > Invert check and return early. Multiple places in this file. > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/draganddrop.js:145: del.invalidate(); > Seems like these two methods should be del.saveDrag(). > > http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... > chrome/browser/resources/ntp/draganddrop.js:181: dragItem.style.right = > 'auto'; > Don't think this line is needed. > > http://codereview.chromium.org/6297013/
On Fri, Jan 21, 2011 at 7:48 PM, <aa@chromium.org> wrote: > On 2011/01/22 00:24:12, jstritar1 wrote: > >> 4. When the apps promo is active, there's ~20px extra margin to the left >> of >> > the > >> web store icon. >> > > That is by design and already happened w/o your change. Did it look like a > bug > to you? It just looked a little strange because the margin isn't there if you have the same apps installed, but the promo is not active. > http://codereview.chromium.org/6297013/ >
I updated this based on your feedback. I also fixed that glitch that occurs when dragging to the right of the detached web store icon. Moving the coordinate knowledge into the delegate really cleaned things up... thanks! http://codereview.chromium.org/6297013/diff/2001/chrome/browser/dom_ui/app_la... File chrome/browser/dom_ui/app_launcher_handler.cc (right): http://codereview.chromium.org/6297013/diff/2001/chrome/browser/dom_ui/app_la... chrome/browser/dom_ui/app_launcher_handler.cc:173: case NotificationType::EXTENSION_LAUNCHER_REORDERED: On 2011/01/22 23:42:57, Aaron Boodman wrote: > Are you sure you need this? I believe that changing the order in ExtensionPrefs > will also cause the PREF_CHANGED notification below to fire. I talked to akalin, and he suggested having a notification to make sync easier to implement. I verified that the PREF_CHANGED notifications aren't triggering multiple re-renders when you reorder the apps. (The apps section only responds to PREF_CHANGED when a launch type changes). http://codereview.chromium.org/6297013/diff/2001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_service.cc (right): http://codereview.chromium.org/6297013/diff/2001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_service.cc:1789: void ExtensionService::SetAppLauncherOrder( On 2011/01/22 23:42:57, Aaron Boodman wrote: > This method seems like it makes more sense directly in ExtensionPrefs to me. Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/new... File chrome/browser/resources/new_new_tab.html (right): http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/new... chrome/browser/resources/new_new_tab.html:289: <script src="ntp/draganddrop.js"></script> On 2011/01/22 00:52:14, arv wrote: > use underscore as word separator in file names Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/new... File chrome/browser/resources/new_new_tab.js (right): http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/new... chrome/browser/resources/new_new_tab.js:620: apps.visible = true; On 2011/01/22 23:42:57, Aaron Boodman wrote: > The visible flag here (and in mostVisited ftm) seems to serve the same purpose > that the css classes are. > > If you rebase, I believe you can just check !classList.contains('disabled') && > !classList.contains('hidden') without having the extra flag. I think we could use the classes to determine visibility, but using this flag helps us avoid invalidating the layout when not necessary. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp/apps.css (right): http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.css:79: .app:not(.dragging) > .app-settings:hover { On 2011/01/22 23:42:57, Aaron Boodman wrote: > If possible, it would be better to have the .dragging class override the .app > class. This way when people add additional rules including .app, they won't have > to remember to do :not(.dragging). > > So for example, if you don't want .app.dragging to have a background image, then > have it declare background-image:none. > > We already have a similar problem with .section:not(.collapsed), but we > shouldn't replicate it. Done. Definitely makes it simpler. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.css:95: -webkit-transition: top .20s, left .20s, right .20s; On 2011/01/22 23:42:57, Aaron Boodman wrote: > It seems that the convention in the rest of the css is .2s (as opposed to .20s). Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.css:121: .app.web-store-entry > a { On 2011/01/22 23:42:57, Aaron Boodman wrote: > Why this change? For the other apps, the app-id attribute is on the <a> inside the app's <div>. Having this one on the <div> made using querySelector in a uniform way difficult, and it seemed to only be used as a class / selector anyways. (The <a> inside the div still has app-id=web-store-entry). http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp/apps.js (right): http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:295: var ids = []; On 2011/01/22 00:52:14, arv wrote: > this.data_ = data.map(function(app) { > return app.id; > }); Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:331: // to be in sync with the rules in apps.css. On 2011/01/22 23:42:57, Aaron Boodman wrote: > Is it possible to get them via offsetHeight/offsetWidth instead? Seems like the > margins could be gottten via getComputedStyle(). Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:374: saveDrag: function() { On 2011/01/22 23:42:57, Aaron Boodman wrote: > Method isn't called from dragdrop.js ? Oops.... that could be a problem! Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:387: layout: function(disable_animations) { On 2011/01/22 23:42:57, Aaron Boodman wrote: > Style nit: I don't much care for the boolean flags to methods. > > You could do: > > layoutWithAnimations() > layoutWithoutAnimations() > > Or: > > layout({disableAnimation:true}); Done. Went with layout({disableAnimation:true}) in case we add more options. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:387: layout: function(disable_animations) { On 2011/01/22 23:42:57, Aaron Boodman wrote: > It sucks that it is different, but naming in JavaScript is disableAnimations for > locals. Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:404: this.dragContainer.setAttribute('launcher-animations', true); On 2011/01/22 00:52:14, arv wrote: > How about introducing a variable to dragContainer. Then you do not need to bind. Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:410: applyAppRects: function() { On 2011/01/22 23:42:57, Aaron Boodman wrote: > Kind of an odd name for this method. What about layoutImpl_? The Impl suffix is > a common convention for a method that does the real work of another public > facing method. Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:419: var t = appsContent.querySelector('[app-id='+apps[i]+']').parentNode; On 2011/01/22 23:42:57, Aaron Boodman wrote: > Avoid single variable letter names unless they are extremely obvious (eg: i, j, > x, y, w, h), or have well-known mathematical meanings (eg: a^2 + b^2 = c^2). Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:429: t.style.right = ''; On 2011/01/22 23:42:57, Aaron Boodman wrote: > It looks like maybe you were going to use 'right', but then decided not to. It > is never set to anything. You can remove this line, and another one in > dragdropcontroller.js. Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:430: t.style.display = this.visible ? '' : 'none'; On 2011/01/22 23:42:57, Aaron Boodman wrote: > If this.visible is false, does it mean that the section is just collapsed, or > that it is completely hidden. > > If completely hidden (eg in menu mode), it seems like doing this layout is not > even necessary. Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:433: innerStyle.left = innerStyle.top = ''; On 2011/01/22 23:42:57, Aaron Boodman wrote: > This seems odd, when are these ever used? Oops... must have brought this in from the most visited drag and drop code. Removed. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:443: var d = this.dimensions; On 2011/01/22 23:42:57, Aaron Boodman wrote: > I don't think it's obvious what d stands for. How about just typing out > 'dimensions'. Or if you make the change I suggested above, you could cache a > dimensions object in the apps object once it is calculated once and just use > that. Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:456: if (rtl) left = availableWidth - left - w; On 2011/01/22 00:52:14, arv wrote: > line break? Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp/draganddrop.js (right): http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2011/01/22 23:42:57, Aaron Boodman wrote: > Seems like this file should be called drag_drop_controller.js. Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:18: // itemHeight, itemWidth, marginWidth, marginHeight, borderWidth On 2011/01/22 23:42:57, Aaron Boodman wrote: > Why does this class care about marginWidth/height borderWidth/height? Why not > just the overall dimensions? I removed the dependency on the dimensions. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:36: this.delegate = delegate; On 2011/01/22 23:42:57, Aaron Boodman wrote: > this.delegate_ ? Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:60: return !!this.delegate.dragItem; On 2011/01/22 23:42:57, Aaron Boodman wrote: > Nit: prefer Boolean(this.delegate.dragItem) or this.delegate.dragItem != null > for clarity. I realized I wasn't using this method, so I yanked it out. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:63: getIndexAt_: function(e) { On 2011/01/22 23:42:57, Aaron Boodman wrote: > Design nit: I think that knowledge of layout should be centralized in the > delegate. By having getIndexAt_ in this class, you are baking in the assumption > that each drag position can be mapped to an index, and that the index is all the > information that the delegate will need in order to decide what to do in > canDropOn(). > > I think it would be more elegant to put getIndexAt_() in apps, and just send the > coordinate in canDropOn(). Leave it up to the delegate to decide what to do with > it. > > I might be missing something that makes this complicated though. Great idea-- I think that's what felt off about the delegate / controller interface. I now pass the coordinates into the canDropOn method and its working a lot nicer. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:64: var del = this.delegate; On 2011/01/22 23:42:57, Aaron Boodman wrote: > Careful with your abbreviations again. Since delegate is only used twice in this > class, this.delegate_ both times is fine IMO. Done. Cleaned this up throughout the class. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:65: var el = del.dragContainer; On 2011/01/22 23:42:57, Aaron Boodman wrote: > This variable is only used once, might as well just have it inline at that time. Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:91: if (item) { On 2011/01/22 23:42:57, Aaron Boodman wrote: > Invert check and return early. Multiple places in this file. Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:145: del.invalidate(); On 2011/01/22 23:42:57, Aaron Boodman wrote: > Seems like these two methods should be del.saveDrag(). Done. http://codereview.chromium.org/6297013/diff/2001/chrome/browser/resources/ntp... chrome/browser/resources/ntp/draganddrop.js:181: dragItem.style.right = 'auto'; On 2011/01/22 23:42:57, Aaron Boodman wrote: > Don't think this line is needed. Done.
On 2011/01/22 00:38:21, arv wrote: > On 2011/01/22 00:24:12, jstritar1 wrote: > > 1. A default drag image appears next to the cursor that I'm not sure how to > > hide. The image isn't too bad on mac and windows, but it's pretty annoying on > > linux. This might be an HTML5 thing.... > > You can set the drag image using dataTransfer.setDragImage. This was not > implemented when I wrote the new tab page but it should work now. Is there a way to get rid of the image altogether with setDragImage? I tried setting it to null and a hidden image, but that didn't work. Maybe a 1px transparent image would work? I prefer moving the element manually (vs using setDragImage) so that the transitions are smoother. The default image also causes some funky :hover styling: 1. Drag an app over another, so they swap positions. 2. Now drop the app outside of the app launcher. 3. The app that moved into the original position will have :hover style once the default image moves there. (I guess the default image is moving to the wrong spot b/c of the way I'm handling drag ends... I'll take another look at it but probably after the branch point).
Very cool. LGTM w/ nits. http://codereview.chromium.org/6297013/diff/16001/chrome/browser/resources/nt... File chrome/browser/resources/ntp/apps.js (right): http://codereview.chromium.org/6297013/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp/apps.js:283: function cssPxToNum(px) { You can just use parseInt() instead of defining this function. http://codereview.chromium.org/6297013/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp/apps.js:337: if (!this.dimensions_) { Invert check and return early. |
