Polish groupedItem button

Fixes #9251 Fixes #9252 Auditors: Test Plan: 1. Open about:styles 2. Click "buttons" 3. Make sure the buttons with 'groupedItem' have margin-left only
brave · Jun 18, 2017 · 3dfc598 · 3dfc598
1 parent c5968c5
commit 3dfc598
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 30 deletions.
diff --git a/app/renderer/components/common/browserButton.js b/app/renderer/components/common/browserButton.js
@@ -84,7 +84,6 @@ const styles = StyleSheet.create({
   browserButton: {
     height: buttonSize,
     width: buttonSize,
-    margin: '0 3px',
     whiteSpace: 'nowrap',
     outline: 'none',
     cursor: 'default',
@@ -104,6 +103,11 @@ const styles = StyleSheet.create({
     fontSize: '13px',
     lineHeight: buttonSize,
 
+    // The buttons as such do not require margin.
+    // They are expected to have margin only if they are grouped.
+    // Also see note on browserButton_groupedItem below.
+    margin: 0,
+
     // cf: https://github.com/brave/browser-laptop/blob/548e11b1c889332fadb379237555625ad2a3c845/less/button.less#L49
     color: globalStyles.button.color,
 
@@ -188,8 +192,19 @@ const styles = StyleSheet.create({
     //
     // TODO: remove !important and check advancedSettings.js
     // once preferences is fully refactored
-    marginRight: '4px !important',
-    marginLeft: '4px !important'
+
+    // 'mm' assures theoretically the equal width of margin
+    // on every platform with any display monitor resolution.
+    // 7.5px = 1/12 inch
+    marginLeft: '7.5px !important',
+
+    // Issue #9252
+    // Because the grouped buttons are *by design* aligned to the right
+    // with flex/grid, margin-right of each one should be null.
+    // If you set margin-right to those buttons, it is expected to lead
+    // to the alignment inconsistency.
+    // Before making a change, please consult with Brad on your proposal.
+    marginRight: 0
   },
 
   browserButton_notificationItem: {

diff --git a/js/about/styles.js b/js/about/styles.js
@@ -282,25 +282,25 @@ class AboutStyle extends ImmutableComponent {
           &lt;BrowserButton subtleItem l10nId='cancel' onClick={'{this.onRemoveBookmark}'} />
         </Code></Pre>
 
-        <BrowserButton primaryColor groupedItem l10nId='primaryColor' onClick={this.onRemoveBookmark} />
-        <BrowserButton secondaryColor groupedItem l10nId='secondaryColor' onClick={this.onRemoveBookmark} />
-        <BrowserButton primaryColor groupedItem l10nId='primaryColor' onClick={this.onRemoveBookmark} />
+        <BrowserButton groupedItem primaryColor l10nId='primaryColor' onClick={this.onRemoveBookmark} />
+        <BrowserButton groupedItem secondaryColor l10nId='secondaryColor' onClick={this.onRemoveBookmark} />
+        <BrowserButton groupedItem primaryColor l10nId='primaryColor' onClick={this.onRemoveBookmark} />
         <Pre><Code>
-          &lt;BrowserButton primaryColor groupedItem l10nId='cancel' onClick={'{this.onRemoveBookmark}'} />{'\n'}
-          &lt;BrowserButton secondaryColor groupedItem l10nId='cancel' onClick={'{this.onRemoveBookmark}'} />{'\n'}
-          &lt;BrowserButton primaryColor groupedItem l10nId='cancel' onClick={'{this.onRemoveBookmark}'} />
+          &lt;BrowserButton groupedItem primaryColor l10nId='cancel' onClick={'{this.onRemoveBookmark}'} />{'\n'}
+          &lt;BrowserButton groupedItem secondaryColor l10nId='cancel' onClick={'{this.onRemoveBookmark}'} />{'\n'}
+          &lt;BrowserButton groupedItem primaryColor l10nId='cancel' onClick={'{this.onRemoveBookmark}'} />
         </Code></Pre>
 
         <BrowserButton extensionItem l10nId='extensionItem' onClick={this.onRemoveBookmark} />
         <Pre><Code>
           &lt;BrowserButton extensionItem l10nId='cancel' onClick={'{this.onRemoveBookmark}'} />
         </Code></Pre>
 
-        <BrowserButton secondaryColor notificationItem groupedItem l10nId='notificationItem' onClick={this.onRemoveBookmark} />
-        <BrowserButton secondaryColor notificationItem groupedItem l10nId='notificationItem' onClick={this.onRemoveBookmark} />
+        <BrowserButton groupedItem secondaryColor notificationItem l10nId='notificationItem' onClick={this.onEnableAutoplay} />
+        <BrowserButton groupedItem secondaryColor notificationItem l10nId='notificationItem' onClick={this.onEnableAutoplay} />
         <Pre><Code>
-          &lt;BrowserButton secondaryColor notificationItem groupedItem l10nId='Yes' onClick={'{this.onRemoveBookmark}'} />{'\n'}
-          &lt;BrowserButton secondaryColor notificationItem groupedItem l10nId='No' onClick={'{this.onRemoveBookmark}'} />
+          &lt;BrowserButton groupedItem secondaryColor notificationItem l10nId='Yes' onClick={'{this.onEnableAutoplay}'} />{'\n'}
+          &lt;BrowserButton groupedItem secondaryColor notificationItem l10nId='No' onClick={'{this.onEnableAutoplay}'} />
         </Code></Pre>
 
         <BrowserButton iconOnly iconClass={globalStyles.appIcons.moreInfo} size='30px' color='rebeccapurple' />
@@ -350,8 +350,8 @@ class AboutStyle extends ImmutableComponent {
                 labore et dolore magna aliqua.
               </CommonFormSection>
               <CommonFormButtonWrapper>
-                <BrowserButton secondaryColor l10nId='Cancel' />
-                <BrowserButton primaryColor l10nId='Done' />
+                <BrowserButton groupedItem secondaryColor l10nId='Cancel' />
+                <BrowserButton groupedItem primaryColor l10nId='Done' />
               </CommonFormButtonWrapper>
               <CommonFormBottomWrapper>
                 <CommonFormClickable>CommonFormClickable</CommonFormClickable>
@@ -395,8 +395,8 @@ class AboutStyle extends ImmutableComponent {
             <Tab2>sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.{'\n'}</Tab2>
             <Tab>&lt;/CommonFormSection&gt;{'\n'}</Tab>
             <Tab>&lt;CommonFormButtonWrapper&gt;{'\n'}</Tab>
-            <Tab2>&lt;button data-l10n-id='Cancel' className='browserButton whiteButton' /&gt;{'\n'}</Tab2>
-            <Tab2>&lt;button data-l10n-id='Done' className='browserButton primaryButton' /&gt;{'\n'}</Tab2>
+            <Tab2>&lt;BrowserButton groupedItem secondaryColor l10nId='Cancel' /&gt;{'\n'}</Tab2>
+            <Tab2>&lt;BrowserButton groupedItem primaryColor l10nId='Done' /&gt;{'\n'}</Tab2>
             <Tab>&lt;/CommonFormButtonWrapper&gt;{'\n'}</Tab>
             <Tab>&lt;CommonFormBottomWrapper&gt;{'\n'}</Tab>
             <Tab2>&lt;CommonFormClickable&gt;CommonFormClickable&lt;/CommonFormClickable&gt;{'\n'}</Tab2>
@@ -541,8 +541,8 @@ class AboutStyle extends ImmutableComponent {
           }}>
             <CommonForm>
               <CommonFormButtonWrapper>
-                <BrowserButton secondaryColor l10nId='Cancel' />
-                <BrowserButton primaryColor l10nId='Done' />
+                <BrowserButton groupedItem secondaryColor l10nId='Cancel' />
+                <BrowserButton groupedItem primaryColor l10nId='Done' />
               </CommonFormButtonWrapper>
             </CommonForm>
           </div>
@@ -555,8 +555,8 @@ class AboutStyle extends ImmutableComponent {
             {'\n'}
             &lt;CommonForm&gt;{'\n'}
             <Tab>&lt;CommonFormButtonWrapper&gt;{'\n'}</Tab>
-            <Tab2>&lt;button data-l10n-id='Cancel' className='browserButton whiteButton' /&gt;{'\n'}</Tab2>
-            <Tab2>&lt;button data-l10n-id='Done' className='browserButton primaryButton' /&gt;{'\n'}</Tab2>
+            <Tab2>&lt;BrowserButton groupedItem secondaryColor l10nId='Cancel' /&gt;{'\n'}</Tab2>
+            <Tab2>&lt;BrowserButton groupedItem primaryColor l10nId='Done' /&gt;{'\n'}</Tab2>
             <Tab>&lt;/CommonFormButtonWrapper&gt;{'\n'}</Tab>
             &lt;/CommonForm&gt;{'\n'}
           </Code></Pre>

diff --git a/less/button.less b/less/button.less
@@ -152,13 +152,4 @@ span.buttonSeparator {
   .settingItem > span + button {
     margin-top: 5px;
   }
-
-  .paymentsContainer,
-  // TODO: refactor button.less to remove this and apply overlayButtonMargin in global.js
-  .syncContainer {
-    .browserButton + .browserButton {
-      margin-left: 8px;
-    }
-  }
-
 }