refactor(textarea): Insert padding-right in order to prevent overlapp…

…ing text and icons
telus · Dec 1, 2017 · d29634c · d29634c
1 parent 3d6c063
commit d29634c
Show file tree

Hide file tree

Showing 12 changed files with 114 additions and 61 deletions.
diff --git a/src/components/Box/Box.modules.scss b/src/components/Box/Box.modules.scss
@@ -1,6 +1,3 @@
-@import '../../scss/settings/responsive-grid';
-@import '../../scss/utility/responsive';
-
 $spacing-base: 1rem; // 16px
 
 $mobile: (

diff --git a/src/components/FormField/FormField.modules.scss b/src/components/FormField/FormField.modules.scss
@@ -79,9 +79,8 @@
 
 /* ICON */
 
-.iconPosition {
-  top: 0.5rem;
-  right: 0.5rem;
+.iconRight {
+  right: 1rem;
 }
 
 /* END ICON */
diff --git a/src/components/FormField/__tests__/addRightPadding.spec.js b/src/components/FormField/__tests__/addRightPadding.spec.js
@@ -0,0 +1,21 @@
+import addRightPadding  from '../addRightPadding'
+
+describe('icon positioning calculations to prevent text overlapping', () => {
+  it('does not add padding when there are no visible icons', () => {
+    const style = addRightPadding(0)
+
+    expect(style).toBeUndefined()
+  })
+
+  it('adds padding for 1 icon', () => {
+    const style = addRightPadding(1)
+
+    expect(style).toEqual({ paddingRight: 'calc(16px + 2rem)' })
+  })
+
+  it('adds padding for 2 icons', () => {
+    const style = addRightPadding(2)
+
+    expect(style).toEqual({ paddingRight: 'calc(32px + 3rem)' })
+  })
+})
diff --git a/src/components/FormField/addRightPadding.js b/src/components/FormField/addRightPadding.js
@@ -0,0 +1,13 @@
+const betweenSpacing = 1 // rem
+const iconWidth = 16 // px
+
+export default numberOfIcons => {
+  if (numberOfIcons === 0) {
+    return undefined
+  }
+
+  return {
+    paddingRight: `calc(${iconWidth * numberOfIcons}px + ${betweenSpacing *
+      (numberOfIcons + 1)}rem)`,
+  }
+}
diff --git a/src/components/Select/Select.jsx b/src/components/Select/Select.jsx
@@ -8,21 +8,25 @@ import DecorativeIcon from '../Icons/DecorativeIcon/DecorativeIcon'
 import Tooltip from '../Tooltip/Tooltip'
 import FormField from '../FormField/FormField'
 import FeedbackIcon from '../FormField/FeedbackIcon'
+import Box from '../Box/Box'
+
+import addRightPadding from '../FormField/addRightPadding'
 
 import styles from './Select.modules.scss'
 import iconWrapperStyles from '../Icons/IconWrapper.modules.scss'
 
 const Select = ({ options, placeholder, ...props }) => (
   <FormField {...props}>
-    {({ className, ...selectProps }, showIcon, feedback) => (
+    {({ className, ...selectProps }, showFeedbackIcon, feedback) => (
       <div className={styles.wrapper}>
         <select
           {...selectProps}
           className={joinClassNames(
             className,
             styles.select,
-            !selectProps.disabled && styles.positionSelectOnTop,
+            !selectProps.disabled && styles.positionSelectOnTop
           )}
+          style={addRightPadding(showFeedbackIcon ? 2 : 1)}
         >
           {placeholder && (
             <option value="" hidden disabled data-no-global-styles>
@@ -37,19 +41,17 @@ const Select = ({ options, placeholder, ...props }) => (
         </select>
 
         {!selectProps.disabled && (
-          <div className={styles.feedbackIconPosition}>
-            <FeedbackIcon showIcon={showIcon} feedback={feedback} />
-          </div>
-        )}
+          <Box inline between={3} dangerouslyAddClassName={styles.iconsPosition}>
+            <FeedbackIcon showIcon={showFeedbackIcon} feedback={feedback} />
 
-        {!selectProps.disabled && (
-          <div className={joinClassNames(iconWrapperStyles.fixLineHeight, styles.caretPosition)}>
-            <DecorativeIcon
-              symbol="caretDown"
-              variant={feedback === 'error' ? 'error' : 'primary'}
-              size={16}
-            />
-          </div>
+            <div className={iconWrapperStyles.fixLineHeight}>
+              <DecorativeIcon
+                symbol="caretDown"
+                variant={feedback === 'error' ? 'error' : 'primary'}
+                size={16}
+              />
+            </div>
+          </Box>
         )}
       </div>
     )}

diff --git a/src/components/Select/Select.modules.scss b/src/components/Select/Select.modules.scss
@@ -26,20 +26,9 @@
   background: transparent;
 }
 
-.feedbackIconPosition {
+.iconsPosition {
   composes: absolute centerVertically from '../Position.modules.scss';
+  composes: iconRight from '../FormField/FormField.modules.scss';
 
   z-index: 1;
-
-  // FIXME: This value is made up.
-  right: 2rem;
-}
-
-.caretPosition {
-  composes: absolute centerVertically from '../Position.modules.scss';
-
-  z-index: 1;
-
-  // FIXME: This value is made up.
-  right: 1rem;
 }
diff --git a/src/components/Select/__tests__/Select.spec.jsx b/src/components/Select/__tests__/Select.spec.jsx
@@ -63,6 +63,18 @@ describe('Select', () => {
     )
   })
 
+  it('positions the select on top of the icons so that it can be clicked', () => {
+    const { findSelectElement } = doMount()
+
+    expect(findSelectElement()).toHaveClassName('positionSelectOnTop')
+  })
+
+  it('positions the down caret so that the text does not overlap it', () => {
+    const { findSelectElement } = doMount()
+
+    expect(findSelectElement()).toHaveStyle('paddingRight', 'calc(16px + 2rem)')
+  })
+
   describe('label', () => {
     it('must have a label', () => {
       const { label } = doMount({ label: 'The label' })
@@ -232,12 +244,12 @@ describe('Select', () => {
       focus()
       expect(findFeedbackIconFade()).toHaveProp('in', false)
     })
-  })
 
-  it('positions the select on top of the icons so that it can be clicked', () => {
-    const { findSelectElement } = doMount()
+    it('ensures that the contents do not overlap the icons', () => {
+      const { findSelectElement } = doMount({ feedback: 'success' })
 
-    expect(findSelectElement()).toHaveClassName('positionSelectOnTop')
+      expect(findSelectElement()).toHaveStyle('paddingRight', 'calc(32px + 3rem)')
+    })
   })
 
   describe('disabling', () => {
@@ -390,7 +402,7 @@ describe('Select', () => {
       style: { color: 'hotpink' },
     })
 
-    expect(findSelectElement()).not.toHaveProp('className', 'my-custom-class')
-    expect(findSelectElement()).not.toHaveProp('style')
+    expect(findSelectElement()).not.toHaveClassName('my-custom-class')
+    expect(findSelectElement()).not.toHaveStyle('color', 'hotpink')
   })
 })
diff --git a/src/components/Select/__tests__/__snapshots__/Select.spec.jsx.snap b/src/components/Select/__tests__/__snapshots__/Select.spec.jsx.snap
@@ -30,6 +30,7 @@ exports[`Select renders 1`] = `
       class="default select positionSelectOnTop"
       data-no-global-styles="true"
       id="the-select"
+      style="padding-right:calc(16px + 2rem)"
     >
       <option
         data-no-global-styles="true"
@@ -45,15 +46,16 @@ exports[`Select renders 1`] = `
       </option>
     </select>
     <div
-      class="feedbackIconPosition"
-    />
-    <div
-      class="fixLineHeight caretPosition"
+      class="betweenRightMarginDesktop-3 inline iconsPosition"
     >
-      <i
-        aria-hidden="true"
-        class="iconCaretDown primary size16"
-      />
+      <div
+        class="fixLineHeight"
+      >
+        <i
+          aria-hidden="true"
+          class="iconCaretDown primary size16"
+        />
+      </div>
     </div>
   </div>
 </div>
@@ -89,6 +91,7 @@ exports[`Select renders with a feedback state and icon 1`] = `
       class="error select positionSelectOnTop"
       data-no-global-styles="true"
       id="the-select"
+      style="padding-right:calc(32px + 3rem)"
     >
       <option
         data-no-global-styles="true"
@@ -104,7 +107,7 @@ exports[`Select renders with a feedback state and icon 1`] = `
       </option>
     </select>
     <div
-      class="feedbackIconPosition"
+      class="betweenRightMarginDesktop-3 inline iconsPosition"
     >
       <div
         style="transition:opacity 100ms ease-in-out;opacity:1"
@@ -118,14 +121,14 @@ exports[`Select renders with a feedback state and icon 1`] = `
           />
         </div>
       </div>
-    </div>
-    <div
-      class="fixLineHeight caretPosition"
-    >
-      <i
-        aria-hidden="true"
-        class="iconCaretDown error size16"
-      />
+      <div
+        class="fixLineHeight"
+      >
+        <i
+          aria-hidden="true"
+          class="iconCaretDown error size16"
+        />
+      </div>
     </div>
   </div>
 </div>

diff --git a/src/components/Textarea/Textarea.jsx b/src/components/Textarea/Textarea.jsx
@@ -8,21 +8,24 @@ import Tooltip from '../Tooltip/Tooltip'
 import FormField from '../FormField/FormField'
 import FeedbackIcon from '../FormField/FeedbackIcon'
 
+import addRightPadding from '../FormField/addRightPadding'
+
 import styles from './Textarea.modules.scss'
 import positionStyles from '../Position.modules.scss'
 
 const Textarea = props => (
   <FormField {...props}>
-    {({ className, ...textareaProps }, showIcon, feedback) => (
+    {({ className, ...textareaProps }, showFeedbackIcon, feedback) => (
       <div className={joinClassNames(positionStyles.relative, styles.preventWidthResizing)}>
         <textarea
           {...textareaProps}
           className={joinClassNames(className, styles.preventWidthResizing)}
+          style={addRightPadding(showFeedbackIcon ? 1 : 0)}
         />
 
         {!textareaProps.disabled && (
           <div className={styles.feedbackIconPosition}>
-            <FeedbackIcon showIcon={showIcon} feedback={feedback} />
+            <FeedbackIcon showIcon={showFeedbackIcon} feedback={feedback} />
           </div>
         )}
       </div>

diff --git a/src/components/Textarea/Textarea.modules.scss b/src/components/Textarea/Textarea.modules.scss
@@ -10,7 +10,7 @@
 
 .feedbackIconPosition {
   composes: absolute from '../Position.modules.scss';
+  composes: iconRight from '../FormField/FormField.modules.scss';
 
   top: 0.5rem;
-  right: 0.5rem;
 }
diff --git a/src/components/Textarea/__tests__/Textarea.spec.jsx b/src/components/Textarea/__tests__/Textarea.spec.jsx
@@ -183,6 +183,7 @@ describe('Textarea', () => {
 
     it('can have an error feedback state', () => {
       const { findTextareaElement, findFeedbackIconFade } = doMount({ feedback: 'error' })
+
       expect(findTextareaElement()).toHaveClassName('error')
       expect(findFeedbackIconFade()).toContainReact(
         <StandaloneIcon
@@ -212,6 +213,18 @@ describe('Textarea', () => {
       focus()
       expect(findFeedbackIconFade()).toHaveProp('in', false)
     })
+
+    it('ensures that the contents do not overlap the icon', () => {
+      const { findTextareaElement } = doMount({ feedback: 'success' })
+
+      expect(findTextareaElement()).toHaveStyle('paddingRight', 'calc(16px + 2rem)')
+    })
+
+    it('does not add any extra padding when there are no icons displayed', () => {
+      const { findTextareaElement } = doMount()
+
+      expect(findTextareaElement()).toHaveProp('style', undefined)
+    })
   })
 
   describe('disabling', () => {
@@ -348,7 +361,7 @@ describe('Textarea', () => {
       style: { color: 'hotpink' },
     })
 
-    expect(findTextareaElement()).not.toHaveProp('className', 'my-custom-class')
-    expect(findTextareaElement()).not.toHaveProp('style')
+    expect(findTextareaElement()).not.toHaveClassName('my-custom-class')
+    expect(findTextareaElement()).not.toHaveStyle('color', 'hotpink')
   })
 })
diff --git a/src/components/Textarea/__tests__/__snapshots__/Textarea.spec.jsx.snap b/src/components/Textarea/__tests__/__snapshots__/Textarea.spec.jsx.snap
@@ -68,6 +68,7 @@ exports[`Textarea renders with a feedback state and icon 1`] = `
       class="error preventWidthResizing"
       data-no-global-styles="true"
       id="the-textarea"
+      style="padding-right:calc(16px + 2rem)"
     />
     <div
       class="feedbackIconPosition"