NTP5: Improving the Tile Page resize logic

chrome/browser/resources/ntp_search/tile_page.js

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 cr.define('ntp', function() {
   'use strict';
 
   //----------------------------------------------------------------------------
+  // Constants
+  //----------------------------------------------------------------------------
+
+  /**
+   * The maximum window height required to show 2 rows of Tiles in the Bottom

[jeremycho_google, 2012/08/23 01:09:28]

minimum

[pedrosimonetti2, 2012/08/24 00:22:15]

Done.

+   * Section. If the height is bigger than this value, 2 rows will be displayed.
+   * @type {Number}
+   * @const
+   */
+  var MAX_WINDOW_HEIGHT = 275;

[jeremycho_google, 2012/08/23 01:09:28]

To me, this sounds like the window can't be larger than this value.  Maybe
TWO_ROW_WINDOW_HEIGHT or TWO_ROW_REQUIRED_HEIGHT or HEIGHT_FOR_TWO_ROWS?

[pedrosimonetti2, 2012/08/24 00:22:15]

Done.

+
+  /**
+   * The minimum window height required to show the Bottom Section. If the
+   * height is smaller than this value, the Bottom Section will be hidden.
+   * @type {Number}
+   * @const
+   */
+  var MIN_WINDOW_HEIGHT = 170;

[jeremycho_google, 2012/08/23 01:09:28]

See above comment, maybe BOTTOM_SECTION_WINDOW_HEIGHT, etc.?

[pedrosimonetti2, 2012/08/24 00:22:15]

Done.

+
+  /**
+   * The maximum window width required to show 5 cols of Tiles in the Bottom

[jeremycho_google, 2012/08/23 01:09:28]

minimum

[pedrosimonetti2, 2012/08/24 00:22:15]

Done.

+   * Section. If the width is bigger than this value, 5 cols will be displayed.

[jeremycho_google, 2012/08/23 01:09:28]

Shouldn't this be used in getColCountForWidth_ then?

[pedrosimonetti2, 2012/08/24 00:22:15]

I don't think so because getColCountForWidth_ will only receive number of
columns that can fit in the max width defined here. So, it will never be higher
than 5 in our case.

+   * @type {Number}
+   * @const
+   */
+  var MAX_WINDOW_WIDTH = 948;

[jeremycho_google, 2012/08/23 01:09:28]

Maybe LARGE_WINDOW_WIDTH?

[jeremycho_google, 2012/08/23 01:09:28]

This value is also used in the bottom section width computation - can you
include a comment for this?

[pedrosimonetti2, 2012/08/24 00:22:15]

Done.

[pedrosimonetti2, 2012/08/24 00:22:15]

I'm not sure if I understand what you mean by "this value is *also* used in the
bottom section width computation". This value is *just* being used for width
computation.

[jeremycho_google, 2012/08/24 00:54:42]

Not done?
On 2012/08/24 00:22:15, pedrosimonetti wrote:

[jeremycho_google, 2012/08/24 00:54:42]

Sorry, what I meant was - can you give a comment explaining what it does in the
width computation?  Seems it should be analogous to the comment for
MIN_BOTTOM_PANEL_WIDTH.

On 2012/08/24 00:22:15, pedrosimonetti wrote:

+
+  /**
+   * The medium window width. If the window width is greater than or equal to
+   * this value, then the Bottom Section width will be the window width minus

[jeremycho_google, 2012/08/23 01:09:28]

Stick with either Bottom Section or Bottom Panel everywhere.

[pedrosimonetti2, 2012/08/24 00:22:15]

Done.

+   * the Bottom Panel's side margin, which we'll call the default width. If the
+   * window is smaller than this value, then the Bottom Section's width will
+   * be an interpolation between the default width, and the minimum width
+   * defined by the constant MIN_BOTTOM_SECTION_WIDTH.
+   * @type {Number}
+   * @const
+   */
+  var MED_WINDOW_WIDTH = 500;
+
+  /**
+   * The minimum window width. If the window is smaller than this value, then
+   * the Bottom Section width will be fixed to MIN_BOTTOM_SECTION_WIDTH.
+   * @type {Number}
+   * @const
+   */
+  var MIN_WINDOW_WIDTH = 300;

[jeremycho_google, 2012/08/23 01:09:28]

Maybe SMALL_WINDOW_WIDTH?

[pedrosimonetti2, 2012/08/24 00:22:15]

Done.

[jeremycho_google, 2012/08/24 00:54:42]

Not done?
On 2012/08/24 00:22:15, pedrosimonetti wrote:

+
+  /**
+   * The minimum Bottom Section width, which will be used when the window width
+   * is smaller than MIN_WINDOW_WIDTH.
+   * @type {Number}
+   * @const
+   */
+  var MIN_BOTTOM_SECTION_WIDTH = 200;
+
+  //----------------------------------------------------------------------------
   // Tile
   //----------------------------------------------------------------------------
 
   /**
    * A virtual Tile class. Each TilePage subclass should have its own Tile
    * subclass implemented too (e.g. MostVisitedPage contains MostVisited
    * tiles, and MostVisited is a Tile subclass).
    * @constructor
    * @param {Object} config TilePage configuration object.
    */
@@ skipping 486 matching lines @@
       var width = colCount * requiredWidth - this.config_.cellMarginStart;
       return width;
     },
 
     /**
      * Gets the bottom panel width.
      * @private
      */
     getBottomPanelWidth_: function() {
       var windowWidth = cr.doc.documentElement.clientWidth;
+      var margin = this.config_.bottomPanelHorizontalMargin;
       var width;
-      // TODO(pedrosimonetti): Add constants?
-      if (windowWidth >= 948)
-        width = 748;
-      else if (windowWidth >= 500)
-        width = windowWidth - 2 * this.config_.bottomPanelHorizontalMargin;
-      else if (windowWidth >= 300)
-        // TODO(pedrosimonetti): Check specification.
-        width = Math.floor(((windowWidth - 300) / 200) * 100 + 200);
+      if (windowWidth >= MAX_WINDOW_WIDTH)
+        width = MAX_WINDOW_WIDTH - 2 * margin;
+      else if (windowWidth >= MED_WINDOW_WIDTH)
+        width = windowWidth - 2 * margin;
+      else if (windowWidth >= MIN_WINDOW_WIDTH)

[jeremycho_google, 2012/08/23 01:09:28]

Add curlies, since this is multi-line?

[pedrosimonetti2, 2012/08/24 00:22:15]

Done.

+        // Interpolation between the previous and next states.
+        width = Math.floor((windowWidth - MIN_WINDOW_WIDTH) /
+            (MED_WINDOW_WIDTH - MIN_WINDOW_WIDTH) *
+            (MED_WINDOW_WIDTH - 2 * margin - MIN_BOTTOM_SECTION_WIDTH) +
+            MIN_BOTTOM_SECTION_WIDTH);
       else
-        width = 200;
+        width = MIN_BOTTOM_SECTION_WIDTH;
 
       return width;
     },
 
     /**
      * Gets the number of available columns.
      * @private
      */
     getAvailableColCount_: function() {
       return this.getColCountForWidth_(this.getBottomPanelWidth_());
@@ skipping 102 matching lines @@
 
       var bottomPanelWidth = this.getBottomPanelWidth_();
       var colCount = this.getColCountForWidth_(bottomPanelWidth);
       var lastColCount = this.colCount_;
       var animatingColCount = this.animatingColCount_;
 
       var windowHeight = cr.doc.documentElement.clientHeight;
 
       this.tileGridContent_.classList.add('animate-tile');
 
-      // TODO(pedrosimonetti): Better handling of height state.
-      // TODO(pedrosimonetti): Add constants?
-      var numOfVisibleRows = windowHeight > 500 ? 2 : 1;
+      this.showBottomSection_(windowHeight < MIN_WINDOW_HEIGHT);
+
+      var numOfVisibleRows = windowHeight > MAX_WINDOW_HEIGHT ? 2 : 1;

[jeremycho_google, 2012/08/23 01:09:28]

When windowHeight < MIN_WINDOW_HEIGHT, should numOfVisibleRows be 0?

[pedrosimonetti2, 2012/08/24 00:22:15]

I thought about this too. Even though zero rows are 'visible' when the entire
Bottom Panel is hidden, what is actually being hidden is the panel, but the rows
themselves remain the way they were before.

In other words, numOfVisibleRows is tracking the number of rows that are being
hidden with CSS. When we hide the Bottom Panel, just the Panel is hidden with
CSS, but the row remains 'visible'. It just turns out that you can't see it ;)

[jeremycho_google, 2012/08/24 00:54:42]

Ok, makes sense.
On 2012/08/24 00:22:15, pedrosimonetti wrote:

       if (numOfVisibleRows != this.numOfVisibleRows_) {
         this.numOfVisibleRows_ = numOfVisibleRows;
         this.paginate_(null, true);
         $('page-list').style.height =
             (this.config_.rowHeight * numOfVisibleRows) + 'px';
       }
 
       // changeVisibleCols
       if (colCount != animatingColCount) {
         var newWidth = this.getWidthForColCount_(colCount);
@@ skipping 38 matching lines @@
 
       this.content_.style.width = bottomPanelWidth + 'px';
 
       this.animatingColCount_ = colCount;
     },
 
     // animation helpers
     // -------------------------------------------------------------------------
 
     /**
+     * Animates the display the Bottom Section.
+     * @param {boolean} show Whether or not to show the Bottom Section.
+     */
+    showBottomSection_: function(show) {

[jeremycho_google, 2012/08/23 01:09:28]

Shouldn't the parameter be 'hide'?  Maybe make this animateBottomSection_
instead?

[pedrosimonetti2, 2012/08/24 00:22:15]

I changed the implementation so it uses a show param, because in order to keep
the code consistent with other methods like showTileRow_() and showTileCols_().

+      $('card-slider-frame').

[jeremycho_google, 2012/08/23 01:09:28]

nit: I think it's more typical to line-break on a '(' instead.

[pedrosimonetti2, 2012/08/24 00:22:15]

Done.

+          classList[show ? 'add' : 'remove']('hide-card-slider');
+    },
+
+    /**
      * 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?
@@ skipping 90 matching lines @@
   }
 
   return {
     // TODO(pedrosimonetti): Drag. Delete after porting the rest of the code.
     getCurrentlyDraggingTile2: deprecate,
     setCurrentDropEffect2: deprecate,
     Tile2: Tile,
     TilePage2: TilePage
   };
 });