fix(fxFlex): apply correct flex-basis stylings (#629)

* fix for when flex-basis is unitless and 0 * fix for when no width/height is applied and flex-basis should be set * fix for IE flex-basis with calc values * fix for SSR properties set to 0 Fixes #277 Fixes #280 Fixes #323 Fixes #528 Fixes #534
angular · Mar 9, 2018 · 1e96cea · 1e96cea
1 parent de7ab76
commit 1e96cea
Show file tree

Hide file tree

Showing 4 changed files with 135 additions and 38 deletions.
diff --git a/src/lib/core/style-utils/style-utils.ts b/src/lib/core/style-utils/style-utils.ts
@@ -70,7 +70,7 @@ export class StyleUtils {
    * Find the DOM element's inline style value (if any)
    */
   lookupInlineStyle(element: HTMLElement, styleName: string): string {
-    return element.style[styleName] || element.style.getPropertyValue(styleName);
+    return element.style[styleName] || element.style.getPropertyValue(styleName) || '';
   }
 
   /**
@@ -88,7 +88,7 @@ export class StyleUtils {
           }
         } else {
           if (this._serverModuleLoaded) {
-            value = `${this._serverStylesheet.getStyleForElement(element, styleName)}`;
+            value = this._serverStylesheet.getStyleForElement(element, styleName);
           }
         }
       }

diff --git a/src/lib/core/stylesheet-map.ts b/src/lib/core/stylesheet-map.ts
@@ -40,8 +40,15 @@ export class StylesheetMap {
   /**
    * Retrieve a given style for an HTML element
    */
-  getStyleForElement(el: HTMLElement, styleName: string): string|number {
+  getStyleForElement(el: HTMLElement, styleName: string): string {
     const styles = this.stylesheet.get(el);
-    return (styles && styles.get(styleName)) || '';
+    let value = '';
+    if (styles) {
+      const style = styles.get(styleName);
+      if (typeof style === 'number' || typeof style === 'string') {
+        value = style + '';
+      }
+    }
+    return value;
   }
 }
diff --git a/src/lib/flex/flex/flex.spec.ts b/src/lib/flex/flex/flex.spec.ts
@@ -142,7 +142,49 @@ describe('flex directive', () => {
       }
     });
 
-    it('should add correct styles for `fxFlex="0%"` usage', () => {
+    it('should add correct styles for flex-basis unitless 0 input', () => {
+      componentWithTemplate(`<div fxFlex="1 1 0"></div>`);
+
+      fixture.detectChanges();
+      expectNativeEl(fixture).toHaveStyle({
+        'flex': '1 1 0%',
+        'box-sizing': 'border-box',
+      }, styler);
+
+      expectNativeEl(fixture).not.toHaveStyle({
+        'max-width': '*'
+      }, styler);
+    });
+
+    it('should add correct styles for flex-basis 0px input', () => {
+      componentWithTemplate(`<div fxFlex="1 1 0px"></div>`);
+
+      fixture.detectChanges();
+      expectNativeEl(fixture).toHaveStyle({
+        'flex': '1 1 0%',
+        'box-sizing': 'border-box',
+      }, styler);
+
+      expectNativeEl(fixture).not.toHaveStyle({
+        'max-width': '*'
+      }, styler);
+    });
+
+    it('should add correct styles for noshrink with basis', () => {
+      componentWithTemplate(`<div fxFlex="1 0 50%"></div>`);
+
+      fixture.detectChanges();
+      expectNativeEl(fixture).toHaveStyle({
+        'flex': '1 0 50%',
+        'box-sizing': 'border-box',
+      }, styler);
+
+      expectNativeEl(fixture).not.toHaveStyle({
+        'max-width': '*'
+      }, styler);
+    });
+
+    it('should add correct styles for `fxFlex="2%"` usage', () => {
       componentWithTemplate(`<div fxFlex='2%'></div>`);
 
       fixture.detectChanges();
@@ -282,7 +324,9 @@ describe('flex directive', () => {
       if (!(platform.FIREFOX || platform.TRIDENT)) {
         expectNativeEl(fixture).toHaveStyle({
           'box-sizing': 'border-box',
-          'flex': '1 1 calc(30vw - 10px)'
+          'flex-grow': '1',
+          'flex-shrink': '1',
+          'flex-basis': 'calc(30vw - 10px)'
         }, styler);
       }
     });
@@ -295,7 +339,9 @@ describe('flex directive', () => {
         setTimeout(() => {
           expectNativeEl(fixture).toHaveStyle({
             'box-sizing': 'border-box',
-            'flex': '1 1 calc(75% - 10px)' // correct version has whitespace
+            'flex-grow': '1',
+            'flex-shrink': '1',
+            'flex-basis': 'calc(75% - 10px)' // correct version has whitespace
           }, styler);
         });
       }

diff --git a/src/lib/flex/flex/flex.ts b/src/lib/flex/flex/flex.ts
@@ -157,11 +157,29 @@ export class FlexDirective extends BaseFxDirective implements OnInit, OnChanges,
     // The flex-direction of this element's flex container. Defaults to 'row'.
     let layout = this._getFlowDirection(this.parentElement, true);
     let direction = (layout.indexOf('column') > -1) ? 'column' : 'row';
-    let css, isValue;
+
+    let max = isFlowHorizontal(direction) ? 'max-width' : 'max-height';
+    let min = isFlowHorizontal(direction) ? 'min-width' : 'min-height';
+
+    let hasCalc = String(basis).indexOf('calc') > -1;
+    let usingCalc = hasCalc || (basis == 'auto');
+    let isPercent = String(basis).indexOf('%') > -1 && !hasCalc;
+    let hasUnits = String(basis).indexOf('px') > -1 || String(basis).indexOf('em') > -1 ||
+      String(basis).indexOf('vw') > -1 || String(basis).indexOf('vh') > -1;
+    let isPx = String(basis).indexOf('px') > -1 || usingCalc;
+
+    let isValue = (hasCalc || hasUnits);
 
     grow = (grow == '0') ? 0 : grow;
     shrink = (shrink == '0') ? 0 : shrink;
 
+    // make box inflexible when shrink and grow are both zero
+    // should not set a min when the grow is zero
+    // should not set a max when the shrink is zero
+    let isFixed = !grow && !shrink;
+
+    let css = {};
+
     // flex-basis allows you to specify the initial/starting main-axis size of the element,
     // before anything else is computed. It can either be a percentage or an absolute value.
     // It is, however, not the breaking point for flex-grow/shrink properties
@@ -182,68 +200,94 @@ export class FlexDirective extends BaseFxDirective implements OnInit, OnChanges,
     };
     switch (basis || '') {
       case '':
-        css = extendObject(clearStyles, {'flex': `${grow} ${shrink} 0.000000001px`});
+        basis = '0.000000001px';
         break;
       case 'initial':   // default
       case 'nogrow':
         grow = 0;
-        css = extendObject(clearStyles, {'flex': '0 1 auto'});
+        basis = 'auto';
         break;
       case 'grow':
-        css = extendObject(clearStyles, {'flex': '1 1 100%'});
+        basis = '100%';
         break;
       case 'noshrink':
         shrink = 0;
-        css = extendObject(clearStyles, {'flex': '1 0 auto'});
+        basis = 'auto';
         break;
       case 'auto':
-        css = extendObject(clearStyles, {'flex': `${grow} ${shrink} auto`});
         break;
       case 'none':
         grow = 0;
         shrink = 0;
-        css = extendObject(clearStyles, {'flex': '0 0 auto'});
+        basis = 'auto';
         break;
       default:
-        let hasCalc = String(basis).indexOf('calc') > -1;
-        let isPercent = String(basis).indexOf('%') > -1 && !hasCalc;
-
-        isValue = hasCalc ||
-            String(basis).indexOf('px') > -1 ||
-            String(basis).indexOf('em') > -1 ||
-            String(basis).indexOf('vw') > -1 ||
-            String(basis).indexOf('vh') > -1;
-
         // Defaults to percentage sizing unless `px` is explicitly set
         if (!isValue && !isPercent && !isNaN(basis as any)) {
           basis = basis + '%';
         }
 
+        // Fix for issue 280
+        if (basis === '0%') {
+          isValue = true;
+        }
+
         if (basis === '0px') {
           basis = '0%';
         }
 
-        css = extendObject(clearStyles, { // fix issue #5345
-          'flex': `${grow} ${shrink} ${isValue ? basis : '100%'}`
-        });
+        // fix issue #5345
+        if (hasCalc) {
+          css = extendObject(clearStyles, {
+            'flex-grow': grow,
+            'flex-shrink': shrink,
+            'flex-basis': isValue ? basis : '100%'
+          });
+        } else {
+          css = extendObject(clearStyles, {
+            'flex': `${grow} ${shrink} ${isValue ? basis : '100%'}`
+          });
+        }
+
         break;
     }
 
-    let max = isFlowHorizontal(direction) ? 'max-width' : 'max-height';
-    let min = isFlowHorizontal(direction) ? 'min-width' : 'min-height';
-
-    let usingCalc = (String(basis).indexOf('calc') > -1) || (basis == 'auto');
-    let isPx = String(basis).indexOf('px') > -1 || usingCalc;
+    if (!(css['flex'] || css['flex-grow'])) {
+      if (hasCalc) {
+        css = extendObject(clearStyles, {
+          'flex-grow': grow,
+          'flex-shrink': shrink,
+          'flex-basis': basis
+        });
+      } else {
+        css = extendObject(clearStyles, {
+          'flex': `${grow} ${shrink} ${basis}`
+        });
+      }
+    }
 
+    // Fix for issues 277 and 534
+    // TODO(CaerusKaru): convert this to just width/height
+    if (basis !== '0%') {
+      css[min] = isFixed || (isPx && grow) ? basis : null;
+      css[max] = isFixed || (!usingCalc && shrink) ? basis : null;
+    }
 
-    // make box inflexible when shrink and grow are both zero
-    // should not set a min when the grow is zero
-    // should not set a max when the shrink is zero
-    let isFixed = !grow && !shrink;
-    css[min] = (basis == '0%') ? 0 : isFixed || (isPx && grow) ? basis : null;
-    css[max] = (basis == '0%') ? 0 : isFixed || (!usingCalc && shrink) ? basis : null;
+    // Fix for issue 528
+    if (!css[min] && !css[max]) {
+      if (hasCalc) {
+        css = extendObject(clearStyles, {
+          'flex-grow': grow,
+          'flex-shrink': shrink,
+          'flex-basis': basis
+        });
+      } else {
+        css = extendObject(clearStyles, {
+          'flex': `${grow} ${shrink} ${basis}`
+        });
+      }
+    }
 
     return extendObject(css, {'box-sizing': 'border-box'});
   }
-
 }