Moves bookmark item width into the state

Resolves brave#9030 Auditors: @bsclifton @bridiver @ayumi Test Plan:
NejcZdovc · Jun 20, 2017 · b132bb2 · b132bb2
1 parent f44726d
commit b132bb2
Show file tree

Hide file tree

Showing 3 changed files with 70 additions and 49 deletions.
diff --git a/app/renderer/components/bookmarks/bookmarksToolbar.js b/app/renderer/components/bookmarks/bookmarksToolbar.js
@@ -22,15 +22,13 @@ const windowStore = require('../../../../js/stores/windowStore')
 // Constants
 const siteTags = require('../../../../js/constants/siteTags')
 const dragTypes = require('../../../../js/constants/dragTypes')
-const {iconSize} = require('../../../../js/constants/config')
 
 // Utils
 const siteUtil = require('../../../../js/state/siteUtil')
 const contextMenus = require('../../../../js/contextMenus')
 const cx = require('../../../../js/lib/classSet')
 const dnd = require('../../../../js/dnd')
 const dndData = require('../../../../js/dndData')
-const {calculateTextWidth} = require('../../../../js/lib/textCalculator')
 
 // Styles
 const globalStyles = require('../styles/global')
@@ -114,39 +112,15 @@ class BookmarksToolbar extends ImmutableComponent {
 
     const noParentItems = this.bookmarks
       .filter((bookmark) => !bookmark.get('parentFolderId'))
-    let widthAccountedFor = 0
     const overflowButtonWidth = 25
-
-    // Dynamically calculate how many bookmark items should appear on the toolbar
-    // before it is actually rendered.
-    this.maxWidth = domUtil.getStyleConstants('bookmark-item-max-width')
-    this.padding = domUtil.getStyleConstants('bookmark-item-padding') * 2
-    // Toolbar padding is only on the left
-    this.toolbarPadding = domUtil.getStyleConstants('bookmarks-toolbar-padding')
-    this.bookmarkItemMargin = domUtil.getStyleConstants('bookmark-item-margin') * 2
-    // No margin for show only favicons
-    this.chevronMargin = domUtil.getStyleConstants('bookmark-item-chevron-margin')
-    this.fontSize = domUtil.getStyleConstants('bookmark-item-font-size')
-    this.fontFamily = domUtil.getStyleConstants('default-font-family')
-    this.chevronWidth = this.chevronMargin + this.fontSize
-    const margin = props.showFavicon && props.showOnlyFavicon ? 0 : this.bookmarkItemMargin
-    widthAccountedFor += this.toolbarPadding
+    const bookmarkItemMargin = domUtil.getStyleConstants('bookmark-item-margin') * 2
+    const margin = props.showFavicon && props.showOnlyFavicon ? 0 : bookmarkItemMargin
+    let widthAccountedFor = domUtil.getStyleConstants('bookmarks-toolbar-padding')
 
     // Loop through until we fill up the entire bookmark toolbar width
     let i
     for (i = 0; i < noParentItems.size; i++) {
-      let iconWidth = props.showFavicon ? iconSize : 0
-      // font-awesome file icons are 3px smaller
-      if (props.showFavicon && !noParentItems.getIn([i, 'folderId']) && !noParentItems.getIn([i, 'favicon'])) {
-        iconWidth -= 3
-      }
-      const chevronWidth = props.showFavicon && noParentItems.getIn([i, 'folderId']) ? this.chevronWidth : 0
-      if (props.showFavicon && props.showOnlyFavicon) {
-        widthAccountedFor += this.padding + iconWidth + chevronWidth
-      } else {
-        const text = noParentItems.getIn([i, 'customTitle']) || noParentItems.getIn([i, 'title']) || noParentItems.getIn([i, 'location'])
-        widthAccountedFor += Math.min(calculateTextWidth(text, `${this.fontSize} ${this.fontFamily}`) + this.padding + iconWidth + chevronWidth, this.maxWidth)
-      }
+      widthAccountedFor += noParentItems.getIn([i, 'bookmarkWidth'])
       widthAccountedFor += margin
       if (widthAccountedFor >= props.windowWidth - overflowButtonWidth) {
         break

diff --git a/docs/state.md b/docs/state.md
@@ -238,6 +238,7 @@ AppStore
   },
   sites: {
     [siteKey]: {
+      bookmarkWidth: number, // width of a bookmark on bookmarks toolbar
       creationTime: number, //creation time of bookmark
       customTitle: string, // User provided title for bookmark; overrides title
       favicon: string, // URL of the favicon

diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js
@@ -3,13 +3,22 @@
 
 'use strict'
 const Immutable = require('immutable')
+
+// State
 const siteCache = require('../../app/common/state/siteCache')
+const {makeImmutable} = require('../../app/common/state/immutableUtil')
+
+// Constants
 const siteTags = require('../constants/siteTags')
 const settings = require('../constants/settings')
-const getSetting = require('../settings').getSetting
+const {iconSize} = require('../constants/config')
+
+// Utils
+const {getSetting} = require('../settings')
 const UrlUtil = require('../lib/urlutil')
 const urlParse = require('../../app/common/urlParse')
-const {makeImmutable} = require('../../app/common/state/immutableUtil')
+const domUtil = require('../../app/renderer/lib/domUtil')
+const {calculateTextWidth} = require('../lib/textCalculator')
 
 const defaultTags = new Immutable.List([siteTags.DEFAULT])
 
@@ -28,14 +37,6 @@ const isBookmarkFolder = (tags) => {
     (tags && typeof tags !== 'string' && tags.includes(siteTags.BOOKMARK_FOLDER))
 }
 
-const isPinnedTab = (tags) => {
-  if (!tags) {
-    return false
-  }
-  return tags.includes(siteTags.PINNED)
-}
-module.exports.isPinnedTab = isPinnedTab
-
 const reorderSite = (sites, order) => {
   sites = sites.map((site) => {
     const siteOrder = site.get('order')
@@ -47,6 +48,43 @@ const reorderSite = (sites, order) => {
   return sites
 }
 
+const isPinnedTab = (tags) => {
+  if (!tags) {
+    return false
+  }
+  return tags.includes(siteTags.PINNED)
+}
+module.exports.isPinnedTab = isPinnedTab
+
+const calcBookmarkWidth = (bookmark) => {
+  let bookmarkWidth = 0
+  // Dynamically calculate how many bookmark items should appear on the toolbar
+  // before it is actually rendered.
+  const maxWidth = domUtil.getStyleConstants('bookmark-item-max-width')
+  const padding = domUtil.getStyleConstants('bookmark-item-padding') * 2
+  // No margin for show only favicons
+  const chevronMargin = domUtil.getStyleConstants('bookmark-item-chevron-margin')
+  const fontSize = domUtil.getStyleConstants('bookmark-item-font-size')
+  const fontFamily = domUtil.getStyleConstants('default-font-family')
+  const chevronWidth = chevronMargin + fontSize
+
+  let iconWidth = props.showFavicon ? iconSize : 0
+  // font-awesome file icons are 3px smaller
+  if (props.showFavicon && !bookmark.get('folderId') && !bookmark.get('favicon')) {
+    iconWidth -= 3
+  }
+  const currentChevronWidth = props.showFavicon && bookmark.get('folderId') ? chevronWidth : 0
+  if (props.showFavicon && props.showOnlyFavicon) {
+    bookmarkWidth += padding + iconWidth + currentChevronWidth
+  } else {
+    const text = bookmark.get('customTitle') || bookmark.get('title') || bookmark.get('location')
+    bookmarkWidth += Math.min(calculateTextWidth(text, `${fontSize} ${fontFamily}`) + padding + iconWidth + currentChevronWidth, maxWidth)
+  }
+
+  return bookmarkWidth
+}
+module.exports.calcBookmarkWidth = calcBookmarkWidth
+
 /**
  * Sort comparator for sort function
  */
@@ -216,6 +254,7 @@ const mergeSiteDetails = (oldSiteDetail, newSiteDetail, tag, folderId, order) =>
     tags,
     objectId: newSiteDetail.get('objectId') || (oldSiteDetail ? oldSiteDetail.get('objectId') : undefined),
     title: newSiteDetail.get('title'),
+    bookmarkWidth: newSiteDetail.get('bookmarkWidth'),
     order
   })
 
@@ -289,18 +328,25 @@ module.exports.addSite = function (state, siteDetail, tag, originalSiteDetail, s
   const oldSite = oldKey !== null ? sites.get(oldKey) : null
   let folderId = siteDetail.get('folderId')
 
-  if (tag === siteTags.BOOKMARK_FOLDER) {
-    if (!oldSite && folderId) {
-      // Remove duplicate folder (needed for import)
-      const dupFolder = sites.find((site) => isBookmarkFolder(site.get('tags')) &&
+  switch (tag) {
+    case siteTags.BOOKMARK_FOLDER: {
+      if (!oldSite && folderId) {
+        // Remove duplicate folder (needed for import)
+        const dupFolder = sites.find((site) => isBookmarkFolder(site.get('tags')) &&
         site.get('parentFolderId') === siteDetail.get('parentFolderId') &&
         site.get('customTitle') === siteDetail.get('customTitle'))
-      if (dupFolder) {
-        state = module.exports.removeSite(state, dupFolder, siteTags.BOOKMARK_FOLDER, true)
+        if (dupFolder) {
+          state = module.exports.removeSite(state, dupFolder, siteTags.BOOKMARK_FOLDER, true)
+        }
+      } else if (!folderId) {
+        // Assign an id if this is a new folder
+        folderId = module.exports.getNextFolderId(sites)
       }
-    } else if (!folderId) {
-      // Assign an id if this is a new folder
-      folderId = module.exports.getNextFolderId(sites)
+      siteDetail = siteDetail.set('bookmarkWidth', calcBookmarkWidth(siteDetail))
+      break
+    }
+    case siteTags.BOOKMARK: {
+      siteDetail = siteDetail.set('bookmarkWidth', calcBookmarkWidth(siteDetail))
     }
   }