Ensure all icons in bookmarks bar are vertically aligned

Fix #13789 Also restore different bookmarks bar height when in text-only mode vs with favicons. Fix #13985 Also reduce flicker of bookmark bar / overflow-indicator / last item by having aphrodite not remove the custom class-name on render.
brave · May 2, 2018 · fe85850 · fe85850
1 parent 4e6cd00
commit fe85850
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 29 deletions.
diff --git a/app/renderer/components/bookmarks/bookmarksToolbar.js b/app/renderer/components/bookmarks/bookmarksToolbar.js
@@ -9,7 +9,6 @@ const Immutable = require('immutable')
 
 // Components
 const ReduxComponent = require('../reduxComponent')
-const BrowserButton = require('../common/browserButton')
 const BookmarkToolbarButton = require('./bookmarkToolbarButton')
 
 // Actions
@@ -196,23 +195,23 @@ class BookmarksToolbar extends React.Component {
   mergeProps (state, ownProps) {
     const currentWindow = state.get('currentWindow')
     const activeFrame = frameStateUtil.getActiveFrame(currentWindow) || Immutable.Map()
+    const bookmarkDisplayMode = bookmarkUtil.showOnlyFavicon()
+        ? 1
+        : bookmarkUtil.showFavicon()
+          ? 2
+          : 3
 
     const props = {}
     // used in renderer
     props.shouldAllowWindowDrag = !isWindows && windowState.shouldAllowWindowDrag(state, currentWindow, activeFrame, isFocused(state))
     props.toolbarBookmarks = bookmarksState.getBookmarksWithFolders(state, 0).take(110).map(item => item.get('key'))
-
+    props.textOnly = (bookmarkDisplayMode === 3)
     // used in other functions
     props.title = activeFrame.get('title')
     props.location = activeFrame.get('location')
     props.showOnlyFavicon = bookmarkUtil.showOnlyFavicon()
     props.showFavicon = bookmarkUtil.showFavicon()
-    props.bookmarkDisplayMode =
-      bookmarkUtil.showOnlyFavicon()
-        ? 1
-        : bookmarkUtil.showFavicon()
-          ? 2
-          : 3
+    props.bookmarkDisplayMode = bookmarkDisplayMode
     return props
   }
 
@@ -223,6 +222,7 @@ class BookmarksToolbar extends React.Component {
     const bookmarkRefs = this.bookmarksToolbarRef.children
     const classNameShowOverflow = css(styles.bookmarksToolbar_hasOverflow)
     this.hiddenBookmarkKeys = null
+    this.hasHiddenKeys = false
     // first check which items overflow with indicator visible
     this.bookmarksToolbarRef.classList.add(classNameShowOverflow)
     // and save which keys were hidden for the overflow menu to open
@@ -235,6 +235,7 @@ class BookmarksToolbar extends React.Component {
       const hiddenKeysWithNoIndicator = getHiddenKeys(bookmarkRefs, this.props.toolbarBookmarks)
       if (hiddenKeysWithNoIndicator && hiddenKeysWithNoIndicator.size) {
         // add overflow indicator as needed
+        this.hasHiddenKeys = true
         this.bookmarksToolbarRef.classList.add(classNameShowOverflow)
       }
     }
@@ -283,11 +284,23 @@ class BookmarksToolbar extends React.Component {
 
   render () {
     this.bookmarkRefs = []
-    return <div className={css(
-      styles.bookmarksToolbar,
-      this.props.shouldAllowWindowDrag && styles.bookmarksToolbar_allowDragging,
-      !this.props.shouldAllowWindowDrag && styles.bookmarksToolbar_disallowDragging
-    )}
+    return <div
+      className={
+        css(
+          styles.bookmarksToolbar,
+          this.props.textOnly && styles.bookmarksToolbar_textOnly,
+          this.props.shouldAllowWindowDrag && styles.bookmarksToolbar_allowDragging,
+          !this.props.shouldAllowWindowDrag && styles.bookmarksToolbar_disallowDragging
+        ) +
+        // Ensure we do not remove overflow indicator on props change
+        // Aphrodite does not support data-attribute selectors :-(
+        // which would be nice to control this functionality.
+        // Instead, we must use a custom class, added by `calculateNonFirstRowItems`
+        // which gets overriden by this `className` attribute here.
+        (this.hasHiddenKeys
+          ? ' ' + css(styles.bookmarksToolbar_hasOverflow)
+          : '')
+      }
       data-test-id='bookmarksToolbar'
       onDrop={this.onDrop}
       onDragEnter={this.onDragEnter}
@@ -303,29 +316,29 @@ class BookmarksToolbar extends React.Component {
             bookmarkKey={bookmarkKey}
           />)
       }
-      <div
+      <button
         className={css(
           styles.bookmarksToolbar__overflowIndicator
         )}
+        data-bookmarks-overflow-indicator
+        onClick={this.onMoreBookmarksMenu}
       >
-        <BrowserButton
-          bookmarksOverflowIndicator
-          iconOnly
-          size={globalStyles.spacing.bookmarksToolbarOverflowButtonWidth}
-          iconClass={globalStyles.appIcons.angleDoubleRight}
-          onClick={this.onMoreBookmarksMenu}
+        <svg className={css(styles.bookmarksToolbar__overflowIndicator__icon)} xmlns='http://www.w3.org/2000/svg'viewBox='0 0 320 512'>
+          <path fill='currentColor' d='M166.9 264.5l-117.8 116c-4.7 4.7-12.3 4.7-17 0l-7.1-7.1c-4.7-4.7-4.7-12.3 0-17L127.3 256 25.1 155.6c-4.7-4.7-4.7-12.3 0-17l7.1-7.1c4.7-4.7 12.3-4.7 17 0l117.8 116c4.6 4.7 4.6 12.3-.1 17zm128-17l-117.8-116c-4.7-4.7-12.3-4.7-17 0l-7.1 7.1c-4.7 4.7-4.7 12.3 0 17L255.3 256 153.1 356.4c-4.7 4.7-4.7 12.3 0 17l7.1 7.1c4.7 4.7 12.3 4.7 17 0l117.8-116c4.6-4.7 4.6-12.3-.1-17z'
         />
-      </div>
+        </svg>
+      </button>
     </div>
   }
 }
 
 const styles = StyleSheet.create({
   bookmarksToolbar: {
     '--bookmarks-toolbar-overflow-indicator-width': '0px',
+    '--bookmarks-toolbar-height': globalStyles.spacing.bookmarksToolbarHeight,
     flex: 1,
     boxSizing: 'border-box',
-    height: globalStyles.spacing.bookmarksToolbarHeight,
+    height: 'var(--bookmarks-toolbar-height)',
     display: 'flex',
     flexDirection: 'row',
     flexWrap: 'wrap',
@@ -341,6 +354,10 @@ const styles = StyleSheet.create({
     position: 'relative'
   },
 
+  bookmarksToolbar_textOnly: {
+    '--bookmarks-toolbar-height': globalStyles.spacing.bookmarksToolbarTextOnlyHeight
+  },
+
   bookmarksToolbar_hasOverflow: {
     '--bookmarks-toolbar-overflow-indicator-visibility': 'visible',
     '--bookmarks-toolbar-overflow-indicator-width': `${globalStyles.spacing.bookmarksToolbarOverflowButtonWidth} !important`
@@ -355,14 +372,29 @@ const styles = StyleSheet.create({
   },
 
   bookmarksToolbar__overflowIndicator: {
+    WebkitAppRegion: 'no-drag',
     position: 'absolute',
     top: 0,
     right: 0,
-    height: globalStyles.spacing.bookmarksToolbarHeight,
+    height: 'var(--bookmarks-toolbar-height)',
     margin: `0 calc(${globalStyles.spacing.bookmarksToolbarPadding} + 5px) 0 auto`,
     visibility: 'var(--bookmarks-toolbar-overflow-indicator-visibility, hidden)',
+    border: 'none',
+    background: 'transparent',
+    padding: 0,
+    width: 'auto',
+    outline: 'none',
     display: 'flex',
-    alignItems: 'center'
+    alignItems: 'center',
+    color: globalStyles.button.color,
+    ':hover': {
+      color: globalStyles.button.default.hoverColor
+    }
+  },
+
+  bookmarksToolbar__overflowIndicator__icon: {
+    width: globalStyles.spacing.bookmarksToolbarOverflowButtonWidth,
+    height: 'auto'
   }
 })
 

diff --git a/app/renderer/components/common/browserButton.js b/app/renderer/components/common/browserButton.js
@@ -61,7 +61,6 @@ class BrowserButton extends ImmutableComponent {
     return <button
       disabled={this.props.disabled}
       data-extension-button={this.props.extensionButton}
-      data-bookmarks-overflow-indicator={this.props.bookmarksOverflowIndicator}
       data-l10n-id={this.props.l10nId}
       data-test-id={this.props.testId}
       data-test2-id={this.props.test2Id}

diff --git a/app/renderer/components/styles/global.js b/app/renderer/components/styles/global.js
@@ -154,8 +154,8 @@ const globalStyles = {
     tabsToolbarHeight: '29px',
     tabPagesHeight: '7px',
     bookmarkHangerMaxWidth: '350px',
-    bookmarksToolbarHeight: '19px',
-    bookmarksToolbarWithFaviconsHeight: '24px',
+    bookmarksToolbarHeight: '20px',
+    bookmarksToolbarTextOnlyHeight: '18px',
     bookmarksFileIconSize: '13px',
     bookmarksFolderIconSize: '15px',
     bookmarksItemMaxWidth: '100px',
@@ -166,7 +166,7 @@ const globalStyles = {
     bookmarksToolbarPadding: '10px',
     bookmarksItemFontSize: '11px',
     bookmarksToolbarButtonDraggingMargin: '25px',
-    bookmarksToolbarOverflowButtonWidth: '14px',
+    bookmarksToolbarOverflowButtonWidth: '9px',
     navbarMenubarMargin: '7px',
     navbarButtonSpacing: '4px',
     navbarButtonWidth: '20px',
@@ -253,7 +253,6 @@ const globalStyles = {
     prefsPanelHeading: '23px'
   },
   appIcons: {
-    angleDoubleRight: 'fa fa-angle-double-right',
     check: 'fa fa-check',
     clipboard: 'fa fa-clipboard',
     closeTab: 'fa fa-times-circle',