Add tweaks to navigationBarButtonContainer.js

- Add containerStyle
brave · Jul 12, 2017 · 561ffbd · 561ffbd
1 parent ab59b14
commit 561ffbd
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 100 deletions.
diff --git a/app/renderer/components/common/browserButton.js b/app/renderer/components/common/browserButton.js
@@ -88,7 +88,14 @@ class NormalizedButton extends ImmutableComponent {
       data-l10n-args={JSON.stringify(this.props.l10nArgs || {})}
       data-button-value={this.props.dataButtonValue}
       onClick={this.props.onClick}
-      className={css(styles.normalizedButton, this.props.custom)}
+      className={css(
+        styles.normalizedButton,
+
+        // For homeButton, stopButton, and bookmarkButton
+        this.props.navigationButton && styles.normalizedButton_navigationButton,
+
+        this.props.custom
+      )}
 
       // for publisherToggle.js
       data-test-authorized={this.props.testAuthorized}
@@ -285,6 +292,15 @@ const styles = StyleSheet.create({
     margin: 0,
     userSelect: 'none',
     whiteSpace: 'nowrap'
+  },
+
+  normalizedButton_navigationButton: {
+    display: 'inline-block',
+    width: '100%',
+    height: '100%',
+
+    // cf: https://github.com/brave/browser-laptop/blob/b161b37cf5e9f59be64855ebbc5d04816bfc537b/less/navigationBar.less#L585
+    padding: 0
   }
 })
 

diff --git a/app/renderer/components/navigation/buttons/bookmarkButton.js b/app/renderer/components/navigation/buttons/bookmarkButton.js
@@ -8,14 +8,14 @@ const ipc = require('electron').ipcRenderer
 
 // Components
 const ReduxComponent = require('../../reduxComponent')
+const {NormalizedButton} = require('../../common/browserButton')
 
 // Actions
 const windowActions = require('../../../../../js/actions/windowActions')
 
 // Constants
 const siteTags = require('../../../../../js/constants/siteTags')
 const messages = require('../../../../../js/constants/messages')
-const settings = require('../../../../../js/constants/settings')
 
 // State
 const tabState = require('../../../../common/state/tabState')
@@ -25,12 +25,10 @@ const frameStateUtil = require('../../../../../js/state/frameStateUtil')
 const windowStore = require('../../../../../js/stores/windowStore')
 
 // Utils
-const cx = require('../../../../../js/lib/classSet')
 const siteUtil = require('../../../../../js/state/siteUtil')
 const UrlUtil = require('../../../../../js/lib/urlutil')
-const {getSetting} = require('../../../../../js/settings')
 
-const {StyleSheet, css} = require('aphrodite/no-important')
+const {StyleSheet} = require('aphrodite/no-important')
 
 const bookmarkButtonIcon = require('../../../../../img/toolbar/bookmark_btn.svg')
 const bookmarkedButtonIcon = require('../../../../../img/toolbar/bookmark_marked.svg')
@@ -97,33 +95,26 @@ class BookmarkButton extends React.Component {
 
   render () {
     return (
-      <button
-        className={cx({
-          // TODO: check if iconOnly solves this and if not
-          // find a way to remove cx cos cx is evooool :P
-          normalizeButton: true,
-          withHomeButton: getSetting(settings.SHOW_HOME_BUTTON),
-          [css(styles.bookmark__button, this.bookmarked && styles.bookmark__button_remove)]: true
-        })}
-        data-l10n-id={
-          this.bookmarked ? 'removeBookmarkButton' : 'addBookmarkButton'
-        }
-        data-test-id={this.bookmarked ? 'bookmarked' : 'notBookmarked'}
+      <NormalizedButton navigationButton
+        custom={[
+          styles.bookmarkButton,
+          this.bookmarked && styles.bookmarkButton_bookmarked
+        ]}
+        l10nId={this.bookmarked ? 'removeBookmarkButton' : 'addBookmarkButton'}
+        testId={this.bookmarked ? 'bookmarked' : 'notBookmarked'}
         onClick={this.onToggleBookmark}
       />
     )
   }
 }
 
 const styles = StyleSheet.create({
-  bookmark__button: {
+  bookmarkButton: {
     background: `url(${bookmarkButtonIcon}) center no-repeat`,
-    backgroundSize: '14px 14px',
-    width: '100%',
-    height: '100%'
+    backgroundSize: '14px 14px'
   },
 
-  bookmark__button_remove: {
+  bookmarkButton_bookmarked: {
     background: `url(${bookmarkedButtonIcon}) center no-repeat`
   }
 })

diff --git a/app/renderer/components/navigation/buttons/homeButton.js b/app/renderer/components/navigation/buttons/homeButton.js
@@ -4,9 +4,11 @@
 
 const React = require('react')
 const Immutable = require('immutable')
+const {StyleSheet} = require('aphrodite/no-important')
 
 // Components
 const ReduxComponent = require('../../reduxComponent')
+const {NormalizedButton} = require('../../common/browserButton')
 
 // Actions
 const appActions = require('../../../../../js/actions/appActions')
@@ -19,13 +21,9 @@ const tabState = require('../../../../common/state/tabState')
 const frameStateUtil = require('../../../../../js/state/frameStateUtil')
 
 // Utils
-const cx = require('../../../../../js/lib/classSet')
 const eventUtil = require('../../../../../js/lib/eventUtil')
 const {getSetting} = require('../../../../../js/settings')
 
-const {StyleSheet, css} = require('aphrodite/no-important')
-const globalStyles = require('../../styles/global')
-
 const homeButtonIcon = require('../../../../../img/toolbar/home_btn.svg')
 
 class HomeButton extends React.Component {
@@ -60,33 +58,20 @@ class HomeButton extends React.Component {
     return props
   }
 
+  // BEM Level: navigationBar__buttonContainer
   render () {
-    return (
-      <button className={cx({
-        // TODO: check if iconOnly solves this and if not
-        // find a way to remove cx cos cx is evooool :P
-        normalizeButton: true,
-        [css(styles.navigationButton, styles.navigationButton_home)]: true
-      })}
-        data-test-id='homeButton'
-        data-l10n-id='homeButton'
-        onClick={this.onHome}
-      />
-    )
+    return <NormalizedButton
+      navigationButton
+      custom={styles.homeButton}
+      testId='homeButton'
+      l10nId='homeButton'
+      onClick={this.onHome}
+    />
   }
 }
 
 const styles = StyleSheet.create({
-  navigationButton: {
-    backgroundColor: globalStyles.color.buttonColor,
-    display: 'inline-block',
-    width: '100%',
-    height: '100%',
-    margin: 0,
-    padding: 0
-  },
-
-  navigationButton_home: {
+  homeButton: {
     background: `url(${homeButtonIcon}) center no-repeat`,
     backgroundSize: '16px 16px'
   }

diff --git a/app/renderer/components/navigation/buttons/navigationBarButtonContainer.js b/app/renderer/components/navigation/buttons/navigationBarButtonContainer.js
@@ -6,20 +6,37 @@ const React = require('react')
 const {StyleSheet, css} = require('aphrodite/no-important')
 const globalStyles = require('../../styles/global')
 
-// TODO: Check if stateless components can benefit from reduxComponent
-// by setting ownProps to stateless props.
+// This component normalizes the wrapper for buttons called on files
+// under app/renderer/components/navigation/buttons/ ro reduce the risk of
+// visual regressions and style inconsistency.
+// Ref https://github.com/brave/browser-laptop/pull/9299#discussion_r124714562
+
+// TODO (Cezar): Check if stateless components can benefit
+// from reduxComponent by setting ownProps to stateless props.
 class NavigationBarButtonContainer extends React.Component {
   render () {
     return (
       <div className={css(
-        this.props.isBoxed && styles.boxed,
-        this.props.isNested && styles.nestedContainer,
-        this.props.isStandalone && styles.standaloneContainer,
-        this.props.onNavigationBarChrome && styles.chromeContainer,
+        // Used for bookmarkButtonContainer, PublisherToggle, noScriptInfo, and UrlBarIcon.
+        this.props.isSquare && styles.container_square,
+
+        // isNested and isStandalone should not be called at the same time
+        // Add border to the bookmark button and publisher button only
+        this.props.isNested && styles.container_nested,
+
+        // Used for stopButton, reloadButton, and homeButton on navigationBar.js
+        // and backButton and forwardButton on navigator.js
+        this.props.isStandalone && styles.container_standalone,
+
+        // Used for stopButton, reloadButton, and homeButton on navigationBar.js
+        // NOT used for the backButton and forwardButton
+        this.props.onNavigationBarChrome && styles.container_chromeButton,
 
         // BEM style class name unique for each component
         this.props.containerFor
-      )}>
+      )}
+        data-test-id={this.props.testId}
+        style={this.props.containerStyle}>
         {this.props.children}
       </div>
     )
@@ -28,9 +45,8 @@ class NavigationBarButtonContainer extends React.Component {
 
 const styles = StyleSheet.create({
   // #9283
-  // Create 25x25 squares and place the buttons at the center of each container
-  // Used for bookmarkButtonContainer, PublisherToggle, noScriptInfo, and UrlBarIcon.
-  boxed: {
+  // Create 25x25 square and place the button at the center of each container
+  container_square: {
     boxSizing: 'border-box',
     display: 'flex',
     alignItems: 'center',
@@ -42,15 +58,12 @@ const styles = StyleSheet.create({
     WebkitAppRegion: 'no-drag'
   },
 
-  // Add border to the bookmark button and publisher button only
-  nestedContainer: {
+  container_nested: {
     border: `1px solid ${globalStyles.color.urlBarOutline}`,
     borderRadius: globalStyles.radius.borderRadiusURL
   },
 
-  // Used for stopButton, reloadButton, and homeButton on navigationBar.js
-  // and backButton and forwardButton on navigator.js
-  standaloneContainer: {
+  container_standalone: {
     display: 'inline-block',
     borderRadius: globalStyles.radius.borderRadiusNavigationButton,
     height: globalStyles.navigationBar.urlbarForm.height,
@@ -62,7 +75,7 @@ const styles = StyleSheet.create({
     }
   },
 
-  chromeContainer: {
+  container_chromeButton: {
     width: globalStyles.navigationBar.navigationButtonContainer.width
   }
 })

diff --git a/app/renderer/components/navigation/buttons/stopButton.js b/app/renderer/components/navigation/buttons/stopButton.js
@@ -3,51 +3,34 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 const React = require('react')
-
-// Utils
-const cx = require('../../../../../js/lib/classSet')
-
-const {StyleSheet, css} = require('aphrodite/no-important')
-const globalStyles = require('../../styles/global')
+const {StyleSheet} = require('aphrodite/no-important')
+const {NormalizedButton} = require('../../common/browserButton')
 
 const stopLoadingButtonIcon = require('../../../../../img/toolbar/stoploading_btn.svg')
 
 class StopButton extends React.Component {
+  // BEM Level: navigationBar__buttonContainer
   render () {
-    return (
-      <button className={cx({
-        // TODO: check if iconOnly solves this and if not
-        // find a way to remove cx cos cx is evooool :P
-        normalizeButton: true,
-        [css(styles.navigationButton, styles.navigationButton_stop)]: true
-      })}
-        data-l10n-id='stopButton'
-        onClick={
-          /*
-          TODO: check if this is ok to ship as-is.
-          This is likely not the best way given it is
-          expecting the property from parent component but this
-          was a quick workaround to avoid dupping code in both navigationbar
-          and this component.
-          */
-          this.props.onStop
-        }
-      />
-    )
+    return <NormalizedButton
+      navigationButton
+      custom={styles.stopButton}
+      l10nid='stopButton'
+      onClick={
+        /*
+        TODO: check if this is ok to ship as-is.
+        This is likely not the best way given it is
+        expecting the property from parent component but this
+        was a quick workaround to avoid dupping code in both navigationbar
+        and this component.
+        */
+        this.props.onStop
+      }
+    />
   }
 }
 
 const styles = StyleSheet.create({
-  navigationButton: {
-    backgroundColor: globalStyles.color.buttonColor,
-    display: 'inline-block',
-    width: '100%',
-    height: '100%',
-    margin: 0,
-    padding: 0
-  },
-
-  navigationButton_stop: {
+  stopButton: {
     background: `url(${stopLoadingButtonIcon}) center no-repeat`,
     backgroundSize: '11px 11px'
   }