Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11564026:
NTP5: Making Apps page taller. (Closed)
|
Created:
8 years ago by pedro (no code reviews) Modified:
8 years ago Reviewers:
Dan Beam CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, estade+watch_chromium.org, arv (Not doing code reviews), pedrosimonetti+watch_chromium.org, Evan Stade, David Black Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionNTP5: Making Apps page taller.
BUG=164977
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173341
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressing Dan's comments #
Total comments: 6
Patch Set 3 : Addressing Dan's comments #
Total comments: 2
Patch Set 4 : NTP5: Making Apps page taller #Patch Set 5 : Fixing regression #Patch Set 6 : NTP5: Making Apps page taller #Patch Set 7 : NTP5: Making Apps page taller #
Messages
Total messages: 26 (0 generated)
Hey Dan, could you please take a look at this one? Finally, another small one!
https://codereview.chromium.org/11564026/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/new_tab.js (right): https://codereview.chromium.org/11564026/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.js:139: var footerHeight; remove this now that it can be a local, not a global https://codereview.chromium.org/11564026/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.js:738: footerHeight = $('bottom-panel-footer').offsetHeight; nit: var footerHeight; (make a local) https://codereview.chromium.org/11564026/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.js:740: headerHeight - footerHeight; or just inline $('bottom-panel-footer').offsetHeight https://codereview.chromium.org/11564026/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.js:1110: function layout(opt_animate) { shouldn't this also take opt_page? why aren't use using the function forward() { newTabView.forward.apply(newTabView, arguments); } forwarding pattern?
https://codereview.chromium.org/11564026/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/new_tab.js (right): https://codereview.chromium.org/11564026/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.js:1110: function layout(opt_animate) { On 2012/12/14 03:44:42, Dan Beam wrote: > shouldn't this also take opt_page? why aren't use using the > > function forward() { > newTabView.forward.apply(newTabView, arguments); > } > > forwarding pattern? why aren't you using**
https://codereview.chromium.org/11564026/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/new_tab.js (right): https://codereview.chromium.org/11564026/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.js:139: var footerHeight; On 2012/12/14 03:44:42, Dan Beam wrote: > remove this now that it can be a local, not a global Done. https://codereview.chromium.org/11564026/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.js:738: footerHeight = $('bottom-panel-footer').offsetHeight; On 2012/12/14 03:44:42, Dan Beam wrote: > nit: var footerHeight; (make a local) I would normally create a variable, just for readability purposes, but since $('bottom-panel-footer').offsetHeight is also pretty readable, I'll accept your second suggestion. https://codereview.chromium.org/11564026/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.js:740: headerHeight - footerHeight; On 2012/12/14 03:44:42, Dan Beam wrote: > or just inline $('bottom-panel-footer').offsetHeight Done. https://codereview.chromium.org/11564026/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.js:1110: function layout(opt_animate) { On 2012/12/14 03:44:42, Dan Beam wrote: > shouldn't this also take opt_page? why aren't use using the > > function forward() { > newTabView.forward.apply(newTabView, arguments); > } > > forwarding pattern? Done. https://codereview.chromium.org/11564026/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.js:1110: function layout(opt_animate) { On 2012/12/14 03:45:02, Dan Beam wrote: > On 2012/12/14 03:44:42, Dan Beam wrote: > > shouldn't this also take opt_page? why aren't use using the > > > > function forward() { > > newTabView.forward.apply(newTabView, arguments); > > } > > > > forwarding pattern? > > why aren't you using** Using the forwarding pattern now.
By the way, thanks a lot for the quick review. I really appreciate it.
almost https://codereview.chromium.org/11564026/diff/2003/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): https://codereview.chromium.org/11564026/diff/2003/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:42: #notification-container.inactive { why not just $('notification-container').hidden from JS? https://codereview.chromium.org/11564026/diff/2003/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:43: -webkit-transition: opacity 200ms; nit: why is this necessary? is this when .inactive is removed that this transition applies now? I don't think transitions work from display: none; right now. https://codereview.chromium.org/11564026/diff/2003/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:45: opacity: 0; nit: why opacity: 0; if display: none;?
https://codereview.chromium.org/11564026/diff/2003/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): https://codereview.chromium.org/11564026/diff/2003/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:42: #notification-container.inactive { On 2012/12/14 05:15:14, Dan Beam wrote: > why not just $('notification-container').hidden from JS? Done. https://codereview.chromium.org/11564026/diff/2003/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:43: -webkit-transition: opacity 200ms; On 2012/12/14 05:15:14, Dan Beam wrote: > nit: why is this necessary? is this when .inactive is removed that this > transition applies now? I don't think transitions work from display: none; > right now. You're right, this is not necessary anymore. We might revisit the animation for closing a notification later. https://codereview.chromium.org/11564026/diff/2003/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:45: opacity: 0; On 2012/12/14 05:15:14, Dan Beam wrote: > nit: why opacity: 0; if display: none;? This CSS rule has been removed.
lgtm w/request https://codereview.chromium.org/11564026/diff/8001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.js (right): https://codereview.chromium.org/11564026/diff/8001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.js:997: window.setTimeout(function() { the setTimeout is no longer needed (was just there to make the transition happen - like I mentioned, there's a bug transitioning from [hidden])
Thanks Dan for the super quick review and approval! I've addressed your comment and synced + rebased. https://codereview.chromium.org/11564026/diff/8001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.js (right): https://codereview.chromium.org/11564026/diff/8001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.js:997: window.setTimeout(function() { On 2012/12/14 05:52:32, Dan Beam wrote: > the setTimeout is no longer needed (was just there to make the transition happen > - like I mentioned, there's a bug transitioning from [hidden]) Oh, okay, now I see what you were saying. The setTimeout was only there because in the previously line it was transitioning from hidden to visible. Now it makes sense.
slgtm (thanks for removing dead code regarding transition end as well)
Hey Dan, I just discovered a regression while doing a series of tests before submitting the CL. I've included a temporary solution now, and I'll start working on a full solution (which will require refactoring the Tile code) in another CL. Since I've changed a few lines since you're approval, please take another look whenever possible.
On a second thought, I'll create another CL with the regression fix, and will submit this one right now.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/11564026/7002
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/11564026/7002
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/11564026/7002
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/11564026/7002
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/11564026/1...
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/11564026/1...
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/11564026/1...
Message was sent while issue was closed.
Change committed as 173341 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
