NTP5: Fine tuning of Apps implementation.

chrome/browser/resources/ntp_search/tile_page.js

Index: chrome/browser/resources/ntp_search/tile_page.js
diff --git a/chrome/browser/resources/ntp_search/tile_page.js b/chrome/browser/resources/ntp_search/tile_page.js
index ca0aa0063d47d736269b6ef3e513c9c82f024542..e6c1c65617ec5d9909ef0acb5b0f5ce1d205f237 100644
--- a/chrome/browser/resources/ntp_search/tile_page.js
+++ b/chrome/browser/resources/ntp_search/tile_page.js
@@ -5,6 +5,18 @@
 cr.define('ntp', function() {
   'use strict';
 
+  /**
+   * @type {number}
+   * @const
+   */
+  var TILE_ROW_HEIGHT = 92;
+
+  /**
+   * @type {number}
+   * @const
+   */
+  var SCROLL_BAR_WIDTH = 12;
+
   //----------------------------------------------------------------------------
   // Tile
   //----------------------------------------------------------------------------
@@ -197,7 +209,6 @@ cr.define('ntp', function() {
   function TilePage() {
     var el = cr.doc.createElement('div');
     el.__proto__ = TilePage.prototype;
-    el.initialize();
 
     return el;
   }
@@ -220,7 +231,9 @@ cr.define('ntp', function() {
       // The start margin of a cell (left or right according to text direction).
       cellMarginStart: 12,
       // The maximum number of Tiles to be displayed.
-      maxTileCount: 6
+      maxTileCount: 6,
+      // Whether the TilePage content will be scrollable.
+      scrollable: false
     },
 
     /**
@@ -229,10 +242,29 @@ cr.define('ntp', function() {
     initialize: function() {
       this.className = 'tile-page';
 
-      // The content defines the actual space a page has to display tiles.
+      // The div that wraps the scrollable element.
+      this.frame_ = this.ownerDocument.createElement('div');
+      this.frame_.className = 'tile-page-frame';
+      this.appendChild(this.frame_);
+
+      // The content/scrollable element.
       this.content_ = this.ownerDocument.createElement('div');
       this.content_.className = 'tile-page-content';
-      this.appendChild(this.content_);
+      this.frame_.appendChild(this.content_);
+
+      if (this.config.scrollable) {
+        this.content_.classList.add('scrollable');
+
+        // The scrollable shadow top.
+        this.shadowTop_ = document.createElement('div');

[Dan Beam, 2012/11/29 04:59:55]

this.ownerDocument.createElement('div') here and below

[pedro (no code reviews), 2012/11/29 08:02:37]

Done.

+        this.shadowTop_.className = 'scrollable-shadow-top';
+        this.content_.appendChild(this.shadowTop_);
+
+        // The scrollable shadow bottom.
+        this.shadowBottom_ = document.createElement('div');
+        this.shadowBottom_.className = 'scrollable-shadow-bottom';
+        this.content_.appendChild(this.shadowBottom_);
+      }
 
       // The div that defines the tile grid viewport.
       this.tileGrid_ = this.ownerDocument.createElement('div');
@@ -253,6 +285,8 @@ cr.define('ntp', function() {
 
       this.tileGrid_.addEventListener('webkitTransitionEnd',
           this.onTileGridTransitionEnd_.bind(this));
+
+      this.content_.addEventListener('scroll', this.onScroll_.bind(this));
     },
 
     /**
@@ -498,7 +532,12 @@ cr.define('ntp', function() {
      * @private
      */
     getColCountForWidth_: function(width) {
-      var availableWidth = width + this.config.cellMarginStart;
+      var content = this.content_;

[Dan Beam, 2012/11/29 04:59:55]

nit: don't really see the point to this var...

[pedro (no code reviews), 2012/11/29 08:02:37]

Var removed.

+      var scrollBarIsVisible = this.config.scrollable &&
+          content.scrollHeight > content.clientHeight;
+      var scrollBarWidth = scrollBarIsVisible ? SCROLL_BAR_WIDTH : 0;
+      var availableWidth = width + this.config.cellMarginStart - scrollBarWidth;
+
       var requiredWidth = this.getTileRequiredWidth_();
       var colCount = Math.floor(availableWidth / requiredWidth);
       return colCount;
@@ -523,12 +562,14 @@ cr.define('ntp', function() {
      */
     getTilePosition_: function(index) {
       var colCount = this.colCount_;
+      var row = Math.floor(index / colCount);
       var col = index % colCount;
       if (isRTL())
         col = colCount - col - 1;
       var config = this.config;
+      var top = TILE_ROW_HEIGHT * row;
       var left = col * (config.cellWidth + config.cellMarginStart);
-      return {top: 0, left: left};
+      return {top: top, left: left};
     },
 
     // rendering
@@ -570,11 +611,6 @@ cr.define('ntp', function() {
           tileGridContent.appendChild(tileRow);
         }
 
-        // Adjust row visibility.
-        var rowVisible = row >= pageOffset &&
-            row <= (pageOffset + numOfVisibleRows - 1);
-        this.showTileRow_(tileRow, rowVisible);
-
         // The tiles inside the current row.
         var tileRowTiles = tileRow.childNodes;
 
@@ -626,6 +662,9 @@ cr.define('ntp', function() {
 
       this.colCount_ = colCount;
       this.rowCount_ = rowCount;
+
+      if (this.onScroll_)

[Dan Beam, 2012/11/29 04:59:55]

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_();

[pedro (no code reviews), 2012/11/29 08:02:37]

Ack.

+        this.onScroll_();
     },
 
     // layout
@@ -681,9 +720,11 @@ cr.define('ntp', function() {
         };
       }
 
-      this.content_.style.width = contentWidth + 'px';
-
       this.animatingColCount_ = colCount;
+
+      this.frame_.style.width = contentWidth + 'px';
+      if (this.onScroll_)
+        this.onScroll_();
     },
 
     // tile repositioning animation
@@ -952,16 +993,7 @@ cr.define('ntp', function() {
     },
 
     /**
-     * Animates the display of a row. TODO(pedrosimonetti): Make it local?
-     * @param {HTMLElement} row The row element.
-     * @param {boolean} show Whether or not to show the row.
-     */
-    showTileRow_: function(row, show) {
-      row.classList[show ? 'remove' : 'add']('hide-row');
-    },
-
-    /**
-     * Animates the display of columns. TODO(pedrosimonetti): Make it local?
+     * Animates the display of columns.
      * @param {number} col The column number.
      * @param {boolean} show Whether or not to show the row.
      */
@@ -978,6 +1010,28 @@ cr.define('ntp', function() {
     // -------------------------------------------------------------------------
 
     /**
+     * Handles the scroll event.
+     * @private

[Dan Beam, 2012/11/29 04:59:55]

definitely make this @protected, this would fail to compile if using closure
compiler.

[pedro (no code reviews), 2012/11/29 08:02:37]

Done.

+     */
+    onScroll_: function(e) {

[Dan Beam, 2012/11/29 04:59:55]

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.
    },
  };

[pedro (no code reviews), 2012/11/29 08:02:37]

Done.

+      if (!this.selected)
+        return;
+
+      // If the TilePage is scrollable, then the opacity of shadow top and
+      // bottom must adjusted, indicating when there's an overflow content.
+      if (this.config.scrollable) {
+        var content = this.content_;
+        var maxGap = 16;
+        var topGap = Math.min(maxGap, content.scrollTop);
+        var bottomGap = Math.min(maxGap, content.scrollHeight -
+            content.scrollTop - content.clientHeight);
+
+        this.shadowTop_.style.opacity = topGap / 16;
+        this.shadowBottom_.style.opacity = bottomGap / 16;
+      }
+    },
+
+    /**
      * Handles the end of the horizontal tile grid transition.
      * @param {Event} e The tile grid webkitTransitionEnd event.
      */