NTP5: Improving the Tile Page resize logic

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 67efaa3067670cb3e84d1d68218cfdf8c9649169..4a13546ba07e41cf2315e86c12fabc1db63d0b19 100644
--- a/chrome/browser/resources/ntp_search/tile_page.js
+++ b/chrome/browser/resources/ntp_search/tile_page.js
@@ -6,6 +6,60 @@ cr.define('ntp', function() {
   'use strict';
 
   //----------------------------------------------------------------------------
+  // Constants
+  //----------------------------------------------------------------------------
+
+  /**
+   * The height required to show 2 rows of Tiles in the Bottom Panel.
+   * @type {Number}

[Dan Beam, 2012/08/25 00:13:29]

@type {number} (number is when you say 5, Number is when you say new Number(5))

[pedrosimonetti2, 2012/08/27 18:52:52]

Done.

+   * @const
+   */
+  var HEIGHT_FOR_TWO_ROWS = 275;
+
+  /**
+   * The height required to show the Bottom Panel.
+   * @type {Number}
+   * @const
+   */
+  var HEIGHT_FOR_BOTTOM_PANEL = 170;
+
+  /**
+   * The Bottom Panel width required to show 5 cols of Tiles, which is used
+   * in the width computation.
+   * @type {Number}
+   * @const
+   */
+  var MAX_BOTTOM_PANEL_WIDTH = 948;

[Dan Beam, 2012/08/25 00:13:29]

why can't these be in CSS as max-width, min-width?

[pedrosimonetti2, 2012/08/27 18:52:52]

Even though I could implement this particularly with CSS (max width), the entire
resizing logic is not possible to be implemented using CSS only. Because of that
I think it's better having all logic in once place, instead of having it
partially implemented with CSS and partially with JavaScript.

+
+  /**
+   * The normal Bottom Panel width. If the window width is greater than or
+   * equal to this value, then the width of the Bottom Panel's content will be
+   * the available width minus side margin. If the available width is smaller
+   * than this value, then the width of the Bottom Panel's content will be an
+   * interpolation between the normal width, and the minimum width defined by
+   * the constant MIN_BOTTOM_PANEL_CONTENT_WIDTH.
+   * @type {Number}
+   * @const
+   */
+  var NORMAL_BOTTOM_PANEL_WIDTH = 500;
+
+  /**
+   * The minimum Bottom Panel width. If the available width is smaller than
+   * this value, then the width of the Bottom Panel's content will be fixed to
+   * MIN_BOTTOM_PANEL_CONTENT_WIDTH.
+   * @type {Number}
+   * @const
+   */
+  var MIN_BOTTOM_PANEL_WIDTH = 300;
+
+  /**
+   * The minimum width of the Bottom Panel's content.
+   * @type {Number}
+   * @const
+   */
+  var MIN_BOTTOM_PANEL_CONTENT_WIDTH = 200;
+
+  //----------------------------------------------------------------------------
   // Tile
   //----------------------------------------------------------------------------
 
@@ -478,17 +532,21 @@ cr.define('ntp', function() {
      */
     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);
-      else
-        width = 200;
+      if (windowWidth >= MAX_BOTTOM_PANEL_WIDTH)

[Dan Beam, 2012/08/25 00:13:29]

nit: curlies when any part of the if/else/else if chain has curlies

[pedrosimonetti2, 2012/08/27 18:52:52]

Done.

+        width = MAX_BOTTOM_PANEL_WIDTH - 2 * margin;
+      else if (windowWidth >= NORMAL_BOTTOM_PANEL_WIDTH)
+        width = windowWidth - 2 * margin;
+      else if (windowWidth >= MIN_BOTTOM_PANEL_WIDTH) {
+        // Interpolation between the previous and next states.
+        width = Math.floor((windowWidth - MIN_BOTTOM_PANEL_WIDTH) /

[Dan Beam, 2012/08/25 00:13:29]

can you use some temporary variables to help document these subexpressions?

[pedrosimonetti2, 2012/08/27 18:52:52]

Done. I reviewed the entire expression again, and included some temporary
variables.

+            (NORMAL_BOTTOM_PANEL_WIDTH - MIN_BOTTOM_PANEL_WIDTH) *
+            (NORMAL_BOTTOM_PANEL_WIDTH -
+                2 * margin - MIN_BOTTOM_PANEL_CONTENT_WIDTH) +
+            MIN_BOTTOM_PANEL_CONTENT_WIDTH);
+      } else
+        width = MIN_BOTTOM_PANEL_CONTENT_WIDTH;
 
       return width;
     },
@@ -612,13 +670,15 @@ cr.define('ntp', function() {
       var shouldAnimate = this.numOfVisibleRows_ !== undefined ||
           this.animatingColCount_ !== undefined;
 
+      this.showBottomPanel_(windowHeight >= HEIGHT_FOR_BOTTOM_PANEL);
+
       // TODO(pedrosimonetti): Add constants?
       // If the number of visible rows has changed, then we need to resize the
       // grid and animate the affected rows. We also need to keep track of
       // whether the number of visible rows has changed because we might have
       // to render the grid when the number of columns hasn't changed.
       var numberOfRowsHasChanged = false;
-      var numOfVisibleRows = windowHeight > 500 ? 2 : 1;
+      var numOfVisibleRows = windowHeight > HEIGHT_FOR_TWO_ROWS ? 2 : 1;
       if (numOfVisibleRows != this.numOfVisibleRows_) {
         this.numOfVisibleRows_ = numOfVisibleRows;
         numberOfRowsHasChanged = true;
@@ -705,6 +765,15 @@ cr.define('ntp', function() {
     // -------------------------------------------------------------------------
 
     /**
+     * Animates the display the Bottom Panel.
+     * @param {boolean} show Whether or not to show the Bottom Panel.
+     */
+    showBottomPanel_: function(show) {
+      $('card-slider-frame').classList[show ? 'remove' : 'add'](
+          '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.