Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11412214:
NTP5: Fine tuning of Apps implementation. (Closed)
|
Created:
8 years ago by pedro (no code reviews) Modified:
8 years ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, estade+watch_chromium.org, arv (Not doing code reviews), pedrosimonetti+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionNTP5: Fine tuning of Apps implementation:
- adjusted styling of App icons
- added scrollable area for Apps
- Apps are now sorted alphabetically
- adjusted positioning of Tiles/Apps
BUG=164018
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170928
Patch Set 1 #
Total comments: 48
Patch Set 2 : Addressing Evan & Dan's comments #Patch Set 3 : sync and rebase #
Total comments: 28
Patch Set 4 : Addressing Evan's comments #Patch Set 5 : Addressing Dan's comments #
Total comments: 13
Patch Set 6 : Addressing Dan's comments #
Total comments: 2
Patch Set 7 : Sync and rebase #Patch Set 8 : Replace rgba(0,0,0,0) with transparent #
Messages
Total messages: 25 (0 generated)
Hey Evan, this is ready for review.
Dan, could you also look over this CL? in particular tile_page.css https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/apps_page.js:616: * |appData|. is this still true? https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/tile_page.css (right): https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:137: left: 0; does this work in rtl https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:159: color-stop(0, rgba(0,0,0,0.1)), color-stop(0.5, rgba(0,0,0,.8)), spaces after commas, everywhere in this file https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:248: border-top: 5px solid transparent; can't this be condensed a bit? something like border: 0 solid transparent; border-top-width: 5px;
https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/apps_page.js:585: scrollable: true nit: hanging "," IMO https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/apps_page.js:615: * Similar to appendApp, but it respects the app_launch_ordinal field of I don't even see where appendApp exists any more... https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/apps_page.js:689: TilePage.prototype.onScroll_.apply(this, arguments); nit: I'd put super calls before if () returns; if possible, also make onScroll @protected instead of @private and remove _. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/new_tab.html (right): https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.html:31: <link rel="stylesheet" href="../shared/css/bubble.css"> rebase issues? https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/tile_page.css (right): https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:132: color-stop(0, rgba(0,0,0,0.1)), color-stop(0.5, rgba(0,0,0,.8)), nit: add spaces between numbers (i.e. rgb(0, 0, 0, 0.1)) https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:133: color-stop(1.0, rgba(0,0,0,0.1))); nit: -webkit-linear-gradient(left, 0% rgba(0, 0, 0, .1), 50% rgba(0, 0, 0, .8), 100% rgba(0, 0, 0, .1)); (or at least I think the syntax can be much less verbose) https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:135: from(rgba(0,0,0,.2)), to(rgba(0,0,0,0))); nit: -webkit-linear-gradient(left, rgba(0, 0, 0, .2), black); https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:138: margin-right: 0; nit: RTL on this as well? https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:146: .scrollable .scrollable-shadow-top:after { nit: ::after (:: is for pseudo-elements, : is for pseudo-selectors, anything that needs content or can be styled as a separate element should be using ::) https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:157: .scrollable .scrollable-shadow-bottom { nit: why do you need the .scrollable- prefix on these classes? https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:207: border-left: 5px solid transparent; nit: I think scrollbars flip to the left in RTL these days... so this may not work in RTL directionality -- have you tried launching chrome in real RTL? https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:220: inset 0 -1px 0 rgba(0,0,0,0.07); nit: I'd line these up -webkit-box-shadow: inset 1px 1px 0 rgba(0, 0, 0, 0.10), inset 0 -1px 0 rgba(0, 0, 0, 0.07); and ditch the extra space on the second line in front of rgba() https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:248: border-top: 5px solid transparent; On 2012/11/29 02:35:47, Evan Stade wrote: > can't this be condensed a bit? something like > > border: 0 solid transparent; > border-top-width: 5px; +1 https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/tile_page.js (right): https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:259: this.shadowTop_ = document.createElement('div'); this.ownerDocument.createElement('div') here and below https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:535: var content = this.content_; nit: don't really see the point to this var... https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:666: if (this.onScroll_) nit: like I already mentioned, this should be protected, so perhaps that's why it's so weird that I see: if (this.someMethodOnlyThisClassCouldDefine_) this.someMethodOnlyThisClassCouldDefine_(); https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:1014: * @private definitely make this @protected, this would fail to compile if using closure compiler. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:1016: onScroll_: function(e) { I prefer: BaseClass.prototype = { /** @protected */ protectedMethod: function() { }, }; then DerivedClass.prototype = { __proto__: BaseClass.prototype, /** @override */ protectedMethod: function() { BaseClass.prototype.protectedMethod.call(this); // optional // Do something cool. }, };
https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/apps_page.js:585: scrollable: true On 2012/11/29 04:59:55, Dan Beam wrote: > nit: hanging "," IMO Done. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/apps_page.js:615: * Similar to appendApp, but it respects the app_launch_ordinal field of On 2012/11/29 04:59:55, Dan Beam wrote: > I don't even see where appendApp exists any more... Correct. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/apps_page.js:616: * |appData|. On 2012/11/29 02:35:47, Evan Stade wrote: > is this still true? Comment has been updated. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/apps_page.js:689: TilePage.prototype.onScroll_.apply(this, arguments); On 2012/11/29 04:59:55, Dan Beam wrote: > nit: I'd put super calls before if () returns; if possible, also make onScroll > @protected instead of @private and remove _. Done. I've also removed the selection check once the element being listened is unique per tile page. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/new_tab.html (right): https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.html:31: <link rel="stylesheet" href="../shared/css/bubble.css"> On 2012/11/29 04:59:55, Dan Beam wrote: > rebase issues? No. I noticed that there's yet another type of notification, which I've never seen happening. It seems that there's an App metadata called notification, so I was trying to test such notifications. Is this type of notification still being used? https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/tile_page.css (right): https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:132: color-stop(0, rgba(0,0,0,0.1)), color-stop(0.5, rgba(0,0,0,.8)), On 2012/11/29 04:59:55, Dan Beam wrote: > nit: add spaces between numbers (i.e. rgb(0, 0, 0, 0.1)) Done. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:133: color-stop(1.0, rgba(0,0,0,0.1))); On 2012/11/29 04:59:55, Dan Beam wrote: > nit: -webkit-linear-gradient(left, 0% rgba(0, 0, 0, .1), > 50% rgba(0, 0, 0, .8), > 100% rgba(0, 0, 0, .1)); > > (or at least I think the syntax can be much less verbose) One thing that makes it verbose is color-stop(...), but AFAIK Chrome requires this syntax. Am I missing something? https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:135: from(rgba(0,0,0,.2)), to(rgba(0,0,0,0))); On 2012/11/29 04:59:55, Dan Beam wrote: > nit: -webkit-linear-gradient(left, rgba(0, 0, 0, .2), black); I guess you meant: -webkit-linear-gradient(top, rgba(0, 0, 0, .2), black); But it turns out we need the black color to be specified as rgba because it will be blended with the mask above. If we use black (rgb), the desired effect won't be achieved. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:137: left: 0; On 2012/11/29 02:35:47, Evan Stade wrote: > does this work in rtl Yes it does once the shadow is symmetrical. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:138: margin-right: 0; On 2012/11/29 04:59:55, Dan Beam wrote: > nit: RTL on this as well? It does too. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:146: .scrollable .scrollable-shadow-top:after { On 2012/11/29 04:59:55, Dan Beam wrote: > nit: ::after (:: is for pseudo-elements, : is for pseudo-selectors, anything > that needs content or can be styled as a separate element should be using ::) Thanks for the explanation. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:157: .scrollable .scrollable-shadow-bottom { On 2012/11/29 04:59:55, Dan Beam wrote: > nit: why do you need the .scrollable- prefix on these classes? The prefix is not actually necessary, so I removed it. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:159: color-stop(0, rgba(0,0,0,0.1)), color-stop(0.5, rgba(0,0,0,.8)), On 2012/11/29 02:35:47, Evan Stade wrote: > spaces after commas, everywhere in this file Done. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:207: border-left: 5px solid transparent; On 2012/11/29 04:59:55, Dan Beam wrote: > nit: I think scrollbars flip to the left in RTL these days... so this may not > work in RTL directionality -- have you tried launching chrome in real RTL? Good catch. I've tried and the scrollbar was misaligned. I'm using -webkit-border-start and end now. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:220: inset 0 -1px 0 rgba(0,0,0,0.07); On 2012/11/29 04:59:55, Dan Beam wrote: > nit: I'd line these up > > -webkit-box-shadow: inset 1px 1px 0 rgba(0, 0, 0, 0.10), > inset 0 -1px 0 rgba(0, 0, 0, 0.07); > > and ditch the extra space on the second line in front of rgba() Done. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:248: border-top: 5px solid transparent; On 2012/11/29 02:35:47, Evan Stade wrote: > can't this be condensed a bit? something like > > border: 0 solid transparent; > border-top-width: 5px; Done. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:248: border-top: 5px solid transparent; On 2012/11/29 04:59:55, Dan Beam wrote: > On 2012/11/29 02:35:47, Evan Stade wrote: > > can't this be condensed a bit? something like > > > > border: 0 solid transparent; > > border-top-width: 5px; > > +1 Done. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/tile_page.js (right): https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:259: this.shadowTop_ = document.createElement('div'); On 2012/11/29 04:59:55, Dan Beam wrote: > this.ownerDocument.createElement('div') here and below Done. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:535: var content = this.content_; On 2012/11/29 04:59:55, Dan Beam wrote: > nit: don't really see the point to this var... Var removed. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:666: if (this.onScroll_) On 2012/11/29 04:59:55, Dan Beam wrote: > nit: like I already mentioned, this should be protected, so perhaps that's why > it's so weird that I see: > > if (this.someMethodOnlyThisClassCouldDefine_) > this.someMethodOnlyThisClassCouldDefine_(); Ack. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:1014: * @private On 2012/11/29 04:59:55, Dan Beam wrote: > definitely make this @protected, this would fail to compile if using closure > compiler. Done. https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:1016: onScroll_: function(e) { On 2012/11/29 04:59:55, Dan Beam wrote: > I prefer: > > BaseClass.prototype = { > /** @protected */ > protectedMethod: function() { > }, > }; > > then > > DerivedClass.prototype = { > __proto__: BaseClass.prototype, > /** @override */ > protectedMethod: function() { > BaseClass.prototype.protectedMethod.call(this); // optional > // Do something cool. > }, > }; Done.
Friendly ping.
https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:620: this.appendTile(new App(appData)); where does the alphabetization happen for new apps (not the initial set of apps) https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.js (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.js:423: // Sort by launch ordinal update https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:132: rgba(0, 0, 0, .1), prefer 0.X for decimals https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:1026: this.shadowTop_.style.opacity = topGap / 16; should these be divided by maxGap?
https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:619: insertApp: function(appData, animate) { I think this is better named "appendApp" as insert* methods generally allow inserting in the middle of a list. https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:682: what happened to if (!this.selected) return; ? https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.js (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.js:420: // Get the array of apps and add any special synthesized entries nit: and end these comments with . https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:132: rgba(0, 0, 0, .1), On 2012/11/30 20:52:46, Evan Stade wrote: > prefer 0.X for decimals (sorry if my example previously said otherwise) https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:140: margin-right: 0; -webkit-margin-end: 0; assuming you want this to flip in RTL https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:170: margin-right: 0; -webkit-margin-end: 0; assuming you want this to flip in RTL https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:242: -webkit-border-end: 0 solid transparent; nit: I'd change these all to border widths, i.e. -webkit-border-end-width: 0; or setting the whole border to none, i.e.: -webkit-border-end: none; https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:254: -webkit-box-shadow: inset 1px 0 0 rgba(0, 0, 0, .10); nit: \s\s -> \s https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:726: if (this.onScroll) how will this ever be false? just this.onScroll(); should suffice, ya? https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:1021: var maxGap = 16; nit: make this a class constant, IMO
Just addressed Evan's comments. I'll address Dan's comments next. https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:620: this.appendTile(new App(appData)); On 2012/11/30 20:52:46, Evan Stade wrote: > where does the alphabetization happen for new apps (not the initial set of apps) Good catch. Added the sorting code back, and changed the sorting criteria to the lowercase title. https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.js (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.js:423: // Sort by launch ordinal On 2012/11/30 20:52:46, Evan Stade wrote: > update Done. https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:132: rgba(0, 0, 0, .1), On 2012/11/30 20:52:46, Evan Stade wrote: > prefer 0.X for decimals Done. https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:1026: this.shadowTop_.style.opacity = topGap / 16; On 2012/11/30 20:52:46, Evan Stade wrote: > should these be divided by maxGap? Yes, they should. Thanks for pointing this out.
https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:619: insertApp: function(appData, animate) { On 2012/11/30 21:58:03, Dan Beam wrote: > I think this is better named "appendApp" as insert* methods generally allow > inserting in the middle of a list. We want to keep the alphabetical order, so I think the insertApp name makes sense. I've adjusted the comment too. https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:682: On 2012/11/30 21:58:03, Dan Beam wrote: > what happened to if (!this.selected) return; ? The DOM element being scrolled here is unique per page, so this means that the page will always be selected when the onScroll handler is called, therefore, checking whether or not the page is selected is not necessary. https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.js (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.js:420: // Get the array of apps and add any special synthesized entries On 2012/11/30 21:58:03, Dan Beam wrote: > nit: and end these comments with . Done. https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:132: rgba(0, 0, 0, .1), On 2012/11/30 21:58:03, Dan Beam wrote: > On 2012/11/30 20:52:46, Evan Stade wrote: > > prefer 0.X for decimals > > (sorry if my example previously said otherwise) No problem. https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:140: margin-right: 0; On 2012/11/30 21:58:03, Dan Beam wrote: > -webkit-margin-end: 0; assuming you want this to flip in RTL Done. https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:170: margin-right: 0; On 2012/11/30 21:58:03, Dan Beam wrote: > -webkit-margin-end: 0; assuming you want this to flip in RTL Done. https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:242: -webkit-border-end: 0 solid transparent; On 2012/11/30 21:58:03, Dan Beam wrote: > nit: I'd change these all to border widths, i.e. > > -webkit-border-end-width: 0; > > or setting the whole border to none, i.e.: > > -webkit-border-end: none; Done. https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:254: -webkit-box-shadow: inset 1px 0 0 rgba(0, 0, 0, .10); On 2012/11/30 21:58:03, Dan Beam wrote: > nit: \s\s -> \s Done. https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:726: if (this.onScroll) On 2012/11/30 21:58:03, Dan Beam wrote: > how will this ever be false? just > > this.onScroll(); > > should suffice, ya? Yes. I originally checked this because onScroll was a AppsPage-only method, but that is not true anymore. Thanks for pointing this out. https://codereview.chromium.org/11412214/diff/7001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:1021: var maxGap = 16; On 2012/11/30 21:58:03, Dan Beam wrote: > nit: make this a class constant, IMO Done.
I wish we could share a little bit more of the CSS in tile_page.css regarding shadow and scrollbar stuff, but I think the selectors you'd have to duplicate would be just as large as the content of the duplicate rules (stupid CSS, :/) anyways, code generally lg to me, I'll let estade@ give the final say though https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/new_tab.html (right): https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.html:31: <link rel="stylesheet" href="../shared/css/bubble.css"> On 2012/11/29 08:02:37, pedrosimonetti wrote: > On 2012/11/29 04:59:55, Dan Beam wrote: > > rebase issues? > > No. I noticed that there's yet another type of notification, which I've never > seen happening. It seems that there's an App metadata called notification, so I > was trying to test such notifications. > > Is this type of notification still being used? estade@: ^ (I think there's a way of triggering notifications from specific apps) https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/tile_page.css (right): https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:135: from(rgba(0,0,0,.2)), to(rgba(0,0,0,0))); On 2012/11/29 08:02:37, pedrosimonetti wrote: > On 2012/11/29 04:59:55, Dan Beam wrote: > > nit: -webkit-linear-gradient(left, rgba(0, 0, 0, .2), black); > > I guess you meant: -webkit-linear-gradient(top, rgba(0, 0, 0, .2), black); > > But it turns out we need the black color to be specified as rgba because it will > be blended with the mask above. If we use black (rgb), the desired effect won't > be achieved. black == rgba(0,0,0,0) - http://www.w3.org/TR/css3-color/#transparent-def https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:611: this.content_.scrollTop = app.tileCell.offsetTop; this might be a bit jerky... have you tried when this changes stuff? https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/mock/mock.js (right): https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... chrome/browser/resources/ntp_search/mock/mock.js:254: } perhaps it doesn't matter if this is mock code, but should you assert(appData); (that you found the app that's about to be removed)? https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:209: -webkit-border-end: 0 solid transparent; nit: same nits re: s/0 solid transparent/none/ (I think it's a bit easier to read what's intended by "none") https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:218: border-bottom: 0 solid transparent; and here https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:249: border: 0 solid transparent; and here https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:675: if (this.onScroll) nit: same nit re: if (this.onScroll)
https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/tile_page.css (right): https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:135: from(rgba(0,0,0,.2)), to(rgba(0,0,0,0))); On 2012/11/30 23:41:45, Dan Beam wrote: > On 2012/11/29 08:02:37, pedrosimonetti wrote: > > On 2012/11/29 04:59:55, Dan Beam wrote: > > > nit: -webkit-linear-gradient(left, rgba(0, 0, 0, .2), black); > > > > I guess you meant: -webkit-linear-gradient(top, rgba(0, 0, 0, .2), black); > > > > But it turns out we need the black color to be specified as rgba because it > will > > be blended with the mask above. If we use black (rgb), the desired effect > won't > > be achieved. > > black == rgba(0,0,0,0) - http://www.w3.org/TR/css3-color/#transparent-def The spec says the color "transparent", AKA "transparent black", is defined by rgba(0, 0, 0, 0). But it does not say about the color "black", which I'm pretty sure it should be 100% opaque and not 100% transparent (otherwise nobody would be able to see it). Anyway, when using black the desired effect doesn't work here. We need both background and mask-box-image colors to have an alpha transparency channel. https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:611: this.content_.scrollTop = app.tileCell.offsetTop; On 2012/11/30 23:41:45, Dan Beam wrote: > this might be a bit jerky... have you tried when this changes stuff? What do you mean by "changes stuff"? I've tested this and it does properly scroll to the proper place, revealing the App that has just been added. https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/mock/mock.js (right): https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... chrome/browser/resources/ntp_search/mock/mock.js:254: } On 2012/11/30 23:41:45, Dan Beam wrote: > perhaps it doesn't matter if this is mock code, but should you assert(appData); > (that you found the app that's about to be removed)? Done. https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:209: -webkit-border-end: 0 solid transparent; On 2012/11/30 23:41:45, Dan Beam wrote: > nit: same nits re: s/0 solid transparent/none/ (I think it's a bit easier to > read what's intended by "none") Done. https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:218: border-bottom: 0 solid transparent; On 2012/11/30 23:41:45, Dan Beam wrote: > and here Done. https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:249: border: 0 solid transparent; On 2012/11/30 23:41:45, Dan Beam wrote: > and here Done. https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:675: if (this.onScroll) On 2012/11/30 23:41:45, Dan Beam wrote: > nit: same nit re: if (this.onScroll) Done.
rgba(0, 0, 0, 0) -> transparent is what I meant https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/tile_page.css (right): https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.css:135: from(rgba(0,0,0,.2)), to(rgba(0,0,0,0))); On 2012/12/01 00:02:29, pedrosimonetti wrote: > On 2012/11/30 23:41:45, Dan Beam wrote: > > On 2012/11/29 08:02:37, pedrosimonetti wrote: > > > On 2012/11/29 04:59:55, Dan Beam wrote: > > > > nit: -webkit-linear-gradient(left, rgba(0, 0, 0, .2), black); > > > > > > I guess you meant: -webkit-linear-gradient(top, rgba(0, 0, 0, .2), black); > > > > > > But it turns out we need the black color to be specified as rgba because it > > will > > > be blended with the mask above. If we use black (rgb), the desired effect > > won't > > > be achieved. > > > > black == rgba(0,0,0,0) - http://www.w3.org/TR/css3-color/#transparent-def > > The spec says the color "transparent", AKA "transparent black", is defined by > rgba(0, 0, 0, 0). > > But it does not say about the color "black", which I'm pretty sure it should be > 100% opaque and not 100% transparent (otherwise nobody would be able to see it). > > Anyway, when using black the desired effect doesn't work here. We need both > background and mask-box-image colors to have an alpha transparency channel. sorry, meant "transparent"
https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:611: this.content_.scrollTop = app.tileCell.offsetTop; On 2012/12/01 00:02:29, pedrosimonetti wrote: > On 2012/11/30 23:41:45, Dan Beam wrote: > > this might be a bit jerky... have you tried when this changes stuff? > > What do you mean by "changes stuff"? > > I've tested this and it does properly scroll to the proper place, revealing the > App that has just been added. sorry, "changes stuff" -> "an app is inserted". if you've tried it and it doesn't look so bad, that's fine, I just imagined it jerking to the new app's spot rather than smoothly scrolling to it (desired, I'd think)
lgtm https://codereview.chromium.org/11412214/diff/11001/chrome/browser/resources/... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11412214/diff/11001/chrome/browser/resources/... chrome/browser/resources/ntp_search/apps_page.js:617: * @param {boolean} animate Whether to animate the insertion. @return
https://codereview.chromium.org/11412214/diff/11001/chrome/browser/resources/... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11412214/diff/11001/chrome/browser/resources/... chrome/browser/resources/ntp_search/apps_page.js:617: * @param {boolean} animate Whether to animate the insertion. On 2012/12/03 18:41:46, Evan Stade wrote: > @return The added App was being returned to we can scroll the page to the proper place when adding a new App, but this does not address the insertion animation. I have another CL being cooked right now which addresses both issues, and does not need returning the Apps here, so I'm removing the return.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/11412214/1...
please wait for lg from all reviewers.
On 2012/11/30 23:41:45, Dan Beam wrote: > I wish we could share a little bit more of the CSS in tile_page.css regarding > shadow and scrollbar stuff, but I think the selectors you'd have to duplicate > would be just as large as the content of the duplicate rules (stupid CSS, :/) > > anyways, code generally lg to me, I'll let estade@ give the final say though ^ lgtm > > https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... > File chrome/browser/resources/ntp_search/new_tab.html (right): > > https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... > chrome/browser/resources/ntp_search/new_tab.html:31: <link rel="stylesheet" > href="../shared/css/bubble.css"> > On 2012/11/29 08:02:37, pedrosimonetti wrote: > > On 2012/11/29 04:59:55, Dan Beam wrote: > > > rebase issues? > > > > No. I noticed that there's yet another type of notification, which I've never > > seen happening. It seems that there's an App metadata called notification, so > I > > was trying to test such notifications. > > > > Is this type of notification still being used? > > estade@: ^ (I think there's a way of triggering notifications from specific > apps) > > https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... > File chrome/browser/resources/ntp_search/tile_page.css (right): > > https://codereview.chromium.org/11412214/diff/1/chrome/browser/resources/ntp_... > chrome/browser/resources/ntp_search/tile_page.css:135: from(rgba(0,0,0,.2)), > to(rgba(0,0,0,0))); > On 2012/11/29 08:02:37, pedrosimonetti wrote: > > On 2012/11/29 04:59:55, Dan Beam wrote: > > > nit: -webkit-linear-gradient(left, rgba(0, 0, 0, .2), black); > > > > I guess you meant: -webkit-linear-gradient(top, rgba(0, 0, 0, .2), black); > > > > But it turns out we need the black color to be specified as rgba because it > will > > be blended with the mask above. If we use black (rgb), the desired effect > won't > > be achieved. > > black == rgba(0,0,0,0) - http://www.w3.org/TR/css3-color/#transparent-def > > https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... > File chrome/browser/resources/ntp_search/apps_page.js (right): > > https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... > chrome/browser/resources/ntp_search/apps_page.js:611: this.content_.scrollTop = > app.tileCell.offsetTop; > this might be a bit jerky... have you tried when this changes stuff? > > https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... > File chrome/browser/resources/ntp_search/mock/mock.js (right): > > https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... > chrome/browser/resources/ntp_search/mock/mock.js:254: } > perhaps it doesn't matter if this is mock code, but should you assert(appData); > (that you found the app that's about to be removed)? > > https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... > File chrome/browser/resources/ntp_search/tile_page.css (right): > > https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... > chrome/browser/resources/ntp_search/tile_page.css:209: -webkit-border-end: 0 > solid transparent; > nit: same nits re: s/0 solid transparent/none/ (I think it's a bit easier to > read what's intended by "none") > > https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... > chrome/browser/resources/ntp_search/tile_page.css:218: border-bottom: 0 solid > transparent; > and here > > https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... > chrome/browser/resources/ntp_search/tile_page.css:249: border: 0 solid > transparent; > and here > > https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... > File chrome/browser/resources/ntp_search/tile_page.js (right): > > https://codereview.chromium.org/11412214/diff/1023/chrome/browser/resources/n... > chrome/browser/resources/ntp_search/tile_page.js:675: if (this.onScroll) > nit: same nit re: if (this.onScroll)
why does this CL have no BUG number?
On 2012/12/03 20:05:26, Evan Stade wrote: > please wait for lg from all reviewers. I thought Dan was happy with your final comment when he stated: "code generally lg to me, I'll let estade@ give the final say though".
On 2012/12/03 20:52:31, Dan Beam wrote: > why does this CL have no BUG number? Added BUG number.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/11412214/5010
Retried try job too often on mac_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/11412214/5010
Message was sent while issue was closed.
Change committed as 170928 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
