Repository: FormidableLabs/spectacle (ts)

Issue:
bgImage prop should also support gradients
I was working on a slide deck when I realized that the `bgImage` prop assumes that the value will be a path to be used with the `url()` function. In many cases, this is a nice assumption, but `background-image` also happens to be the CSS property that gradients are added with.

I would like to add support for being able to add gradients to slides. I think this can be done one of two ways.

1) We modify `bgImage` prop to expect a string without automatically wrapping the string in a `url()` call. The challenge with this approach is coming up with a way to handle the use case where a user may purposefully, though more likely inadvertently, use the `bgDarken` prop as well. We could ignore `bgDarken` if the string supplied to `bgImage` has a gradient in it.

2) Another option is to add a new prop, `bgGradient` that handles this one use case. Challenges with this will be deciding how to handle situations where all of the props (`bgImage`, `bgDarken`, and `bgGradient`) are simultaneously present, but we could come to a decision on that. This might be the cleaner of the two implementations.

I'm open to other ideas as well and happy to do the work of the PR, but wanted to get some input before going down one path or another.

Pull request description:
Move font-size and warn utils out of base, refactor conditionals, add bgImageStyle
*bgImageStyle prop allows for setting the backgroundImage property directly (resolve #428)
*bgLighten prop allows for setting opacity with white instead black  (based on #531, happy to rebase/refactor for the bgLighten warning)
*Refactor for greater purity/granularity/testability
*Disabled consistent-return for base.js. This seems like an Airbnb rule that's part of a larger discussion about expectations of implicit returns, but would add a lot of boilerplate here.
*Validated that old and new getStyles functions return the same objects/trigger the same one time warnings but some tests would still be a good idea.


Patch:
diff --git a/.eslintrc b/.eslintrc
index 16ba8c5..e660aef 100644
--- a/.eslintrc
+++ b/.eslintrc
@@ -42,6 +42,7 @@
     "func-style": "off",
     "arrow-parens": "off",
     "no-use-before-define": "off",
+    "no-undef": ["error", { "typeof": true }],
     "react/jsx-filename-extension": "off",
     "react/require-extension": "off",
     "react/no-multi-comp": "off",
diff --git a/src/utils/base.js b/src/utils/base.js
index f33e7f5..191fd5b 100644
--- a/src/utils/base.js
+++ b/src/utils/base.js
@@ -1,175 +1,169 @@
-/*eslint max-statements:0,complexity:0,no-invalid-this:0*/
-
-const parseFontSize = function(fontSize) {
-  const sizeComponents = fontSize.match(/\d*\.*\d+|\D+/g);
-  const size = parseFloat(sizeComponents[0]);
-  const unit = sizeComponents[1];
-  return { size, unit };
-};
-
-const getFontSizeFromElement = function(element) {
-  const fontSize = window.getComputedStyle
-    ? window.getComputedStyle(element).getPropertyValue('font-size')
-    : element.currentStyle.fontSize;
-  return fontSize ? parseFontSize(fontSize) : null;
+/*eslint max-statements:0,complexity:0,no-invalid-this:0,consistent-return:0*/
+import checkWarnings from './warn';
+// eslint-disable-next-line max-params
+export const buildStyles = (transforms, props, context, styles = {}) => {
+  return transforms.reduce((av, cv) => {
+    return { ...av, ...cv(props, context, av) };
+  }, styles);
 };
 
-const convertFontSizeToPx = function(fontSize) {
-  let convertedFontSize;
-
-  if (typeof fontSize === 'number') {
-    convertedFontSize = fontSize;
-  } else if (typeof fontSize === 'string') {
-    const parsedFont = parseFontSize(fontSize);
-    const bodyFont = getFontSizeFromElement(document.body);
-    const htmlFont = getFontSizeFromElement(document.documentElement);
-
-    switch (parsedFont.unit) {
-      case 'px':
-        convertedFontSize = parsedFont.size;
-        break;
-      case 'pt':
-        convertedFontSize = (parsedFont.size * 96) / 72;
-        break;
-      case '%':
-        if (bodyFont) {
-          convertedFontSize = (bodyFont.size * parsedFont.size) / 100;
-        }
-        break;
-      case 'em':
-        if (bodyFont) {
-          convertedFontSize = bodyFont.size * parsedFont.size;
-        }
-        break;
-      case 'rem':
-        if (htmlFont) {
-          convertedFontSize = htmlFont.size * parsedFont.size;
-        }
-        break;
-      default:
-        convertedFontSize = parsedFont.size;
-    }
-  }
-  return convertedFontSize;
-};
-
-export const getStyles = function getStyles() {
-  if (
-    process.env.NODE_ENV !== 'production' &&
-    typeof this.warnedAboutFontSize === 'undefined'
-  ) {
-    this.warnedAboutFontSize = false;
-  }
-
-  const {
-    italic,
-    bold,
-    caps,
-    margin,
-    padding,
-    textColor,
-    textFont,
-    textSize,
-    textAlign,
-    bgColor,
-    bgGradient,
-    bgImage,
-    bgDarken,
-    bgSize,
-    bgPosition,
-    bgRepeat,
-    overflow,
-    height
-  } = this.props;
-
-  const styles = {};
-  const recommendedMinFontSizePx = 24;
-
-  if (typeof italic === 'boolean') {
-    styles.fontStyle = italic ? 'italic' : 'normal';
-  }
-  if (typeof bold === 'boolean') {
-    styles.fontWeight = bold ? 'bold' : 'normal';
-  }
-  if (typeof caps === 'boolean') {
-    styles.textTransform = caps ? 'uppercase' : 'none';
-  }
-  if (margin) {
-    styles.margin = margin;
-  }
-  if (padding) {
-    styles.padding = padding;
-  }
+const applyMargin = ({ margin }) => (margin ? { margin } : undefined);
+const applyPadding = ({ padding }) => (padding ? { padding } : undefined);
+const applyOverflow = ({ overflow }) => (overflow ? { overflow } : undefined);
+const applyHeight = ({ height }) => (height ? { height } : undefined);
 
+export const transformTextColor = ({ textColor }, context) => {
   if (textColor) {
     let color = '';
-    if (!this.context.styles.colors.hasOwnProperty(textColor)) {
+    if (!context.styles.colors.hasOwnProperty(textColor)) {
       color = textColor;
     } else {
-      color = this.context.styles.colors[textColor];
+      color = context.styles.colors[textColor];
     }
-    styles.color = color;
+    return { color };
   }
+};
+
+export const transformTextFont = ({ textFont }, context) => {
   if (textFont) {
-    let font = '';
-    if (!this.context.styles.fonts.hasOwnProperty(textFont)) {
-      font = textFont;
+    let fontFamily = '';
+    if (!context.styles.fonts.hasOwnProperty(textFont)) {
+      fontFamily = textFont;
     } else {
-      font = this.context.styles.fonts[textFont];
+      fontFamily = context.styles.fonts[textFont];
     }
-    styles.fontFamily = font;
+    return { fontFamily };
+  }
+};
+
+export const transformTextAlign = ({ textAlign }) => {
+  if (textAlign) {
+    return { textAlign };
   }
+};
+export function transformTextSize({ textSize }) {
   if (textSize) {
-    styles.fontSize = textSize;
-    if (
-      process.env.NODE_ENV !== 'production' &&
-      !this.warnedAboutFontSize &&
-      this.context.store.getState().style.globalStyleSet
-    ) {
-      const fontSize =
-        convertFontSizeToPx(textSize) || recommendedMinFontSizePx;
-      if (fontSize < recommendedMinFontSizePx) {
-        // eslint-disable-next-line
-        console.warn(
-          `prop \`textSize="${textSize}"\` is below the recommended minimum of ${recommendedMinFontSizePx}px`
-        );
-        this.warnedAboutFontSize = true;
-      }
-    }
+    return { fontSize: textSize };
   }
-  if (textAlign) {
-    styles.textAlign = textAlign;
+}
+export const transformItalic = ({ italic }) => {
+  if (typeof italic === 'boolean') {
+    return { fontStyle: italic ? 'italic' : 'normal' };
+  }
+};
+
+export const transformBold = ({ bold }) => {
+  if (typeof bold === 'boolean') {
+    return { fontWeight: bold ? 'bold' : 'normal' };
+  }
+};
+export const transformCaps = ({ caps }) => {
+  if (typeof caps === 'boolean') {
+    return { textTransform: caps ? 'uppercase' : 'none' };
   }
+};
+export const transformBgColor = ({ bgColor }, context) => {
   if (bgColor) {
-    let color = '';
-    if (!this.context.styles.colors.hasOwnProperty(bgColor)) {
-      color = bgColor;
+    let backgroundColor = '';
+    if (!context.styles.colors.hasOwnProperty(bgColor)) {
+      backgroundColor = bgColor;
     } else {
-      color = this.context.styles.colors[bgColor];
+      backgroundColor = context.styles.colors[bgColor];
     }
-    styles.backgroundColor = color;
+    return { backgroundColor };
   }
-  if (bgGradient) {
-    styles.backgroundImage = bgGradient;
+};
+
+export const transformBgSize = ({ bgImage, bgSize }) => {
+  if (bgImage) {
+    return { backgroundSize: bgSize || 'cover' };
   }
+};
 
+export const transformBgImageByGradient = ({ bgGradient }) => {
+  return { backgroundImage: bgGradient };
+};
+
+export const transformBgPosition = ({ bgImage, bgPosition }) => {
   if (bgImage) {
-    if (bgDarken) {
-      styles.backgroundImage = `linear-gradient( rgba(0, 0, 0, ${bgDarken}), rgba(0, 0, 0, ${bgDarken}) ), url(${bgImage})`;
-    } else {
-      styles.backgroundImage = `url(${bgImage})`;
-    }
-    styles.backgroundSize = bgSize || 'cover';
-    styles.backgroundPosition = bgPosition || 'center center';
-    if (bgRepeat) {
-      styles.backgroundRepeat = bgRepeat;
-    }
+    return { backgroundPosition: bgPosition || 'center center' };
   }
-  if (overflow) {
-    styles.overflow = overflow;
+};
+export const transformBgRepeat = ({ bgImage, bgRepeat }) => {
+  if (bgImage) {
+    return { backgroundRepeat: bgRepeat };
   }
-  if (height) {
-    styles.height = height;
+};
+export const transformBgImageByBgStyle = ({ bgImageStyle }) => {
+  return { backgroundImage: bgImageStyle };
+};
+export const transformBgImage = ({
+  bgImage,
+  bgDarken,
+  bgLighten,
+  bgImageStyle,
+  bgGradient
+}) => {
+  if (!bgImage) {
+    return;
   }
-  return styles;
+
+  if (bgImageStyle) {
+    return transformBgImageByBgStyle({ bgImageStyle });
+  }
+
+  if (bgGradient) {
+    return transformBgImageByGradient({ bgGradient });
+  }
+
+  if (bgDarken) {
+    return {
+      backgroundImage: `linear-gradient( rgba(0, 0, 0, ${bgDarken}), rgba(0, 0, 0, ${bgDarken}) ), url(${bgImage})`
+    };
+  } else if (bgLighten) {
+    return {
+      backgroundImage: `linear-gradient( rgba(255, 255, 255, ${bgLighten}), rgba(255, 255, 255, ${bgLighten}) ), url(${bgImage})`
+    };
+  } else {
+    return { backgroundImage: `url(${bgImage})` };
+  }
+};
+
+const textTransforms = [
+  transformItalic,
+  transformBold,
+  transformCaps,
+  transformTextColor,
+  transformTextFont,
+  transformTextAlign,
+  transformTextSize
+];
+
+export const generalTransforms = [
+  applyMargin,
+  applyPadding,
+  applyOverflow,
+  applyHeight
+];
+
+export const bgTransforms = [
+  transformBgColor,
+  transformBgImage,
+  transformBgRepeat,
+  transformBgSize,
+  transformBgPosition
+];
+
+const styleTransforms = [
+  ...textTransforms,
+  ...generalTransforms,
+  ...bgTransforms
+];
+
+export const getStyles = function getStyles() {
+  if (process.env.NODE_ENV !== 'production') {
+    checkWarnings(this);
+  }
+
+  return buildStyles(styleTransforms, this.props, this.context);
 };
diff --git a/src/utils/font-size.js b/src/utils/font-size.js
new file mode 100644
index 0000000..daf0330
--- /dev/null
+++ b/src/utils/font-size.js
@@ -0,0 +1,50 @@
+export const parseFontSize = function(fontSize) {
+  const sizeComponents = fontSize.match(/\d*\.*\d+|\D+/g);
+  const size = parseFloat(sizeComponents[0]);
+  const unit = sizeComponents[1];
+  return { size, unit };
+};
+
+export const getFontSizeFromElement = function(element) {
+  const fontSize = window.getComputedStyle
+    ? window.getComputedStyle(element).getPropertyValue('font-size')
+    : element.currentStyle.fontSize;
+  return fontSize ? parseFontSize(fontSize) : null;
+};
+
+export const convertFontSizeToPx = function(fontSize) {
+  let convertedFontSize;
+
+  if (typeof fontSize === 'number') {
+    convertedFontSize = fontSize;
+  } else if (typeof fontSize === 'string') {
+    const parsedFont = parseFontSize(fontSize);
+    const bodyFont = getFontSizeFromElement(document.body);
+    const htmlFont = getFontSizeFromElement(document.documentElement);
+
+    switch (parsedFont.unit) {
+      case 'px':
+        convertedFontSize = parsedFont.size;
+        break;
+      case 'pt':
+        convertedFontSize = (parsedFont.size * 96) / 72;
+        break;
+      case '%':
+        if (bodyFont) {
+          convertedFontSize = (bodyFont.size * parsedFont.size) / 100;
+        }
+        break;
+      case 'em':
+        if (bodyFont) {
+          convertedFontSize = bodyFont.size * parsedFont.size;
+        }
+        break;
+      case 'rem':
+        if (htmlFont) {
+          convertedFontSize = htmlFont.size * parsedFont.size;
+        }
+        break;
+    }
+  }
+  return convertedFontSize;
+};
diff --git a/src/utils/warn.js b/src/utils/warn.js
new file mode 100644
index 0000000..5e55eea
--- /dev/null
+++ b/src/utils/warn.js
@@ -0,0 +1,61 @@
+import { convertFontSizeToPx } from './font-size';
+import { get } from 'lodash';
+
+const recommendedMinFontSizePx = 24;
+
+const fontSizeWarning = component => {
+  const { props, context } = component;
+
+  if (context.store && !get(context.store.getState(), 'style.globalStyleSet')) {
+    return false;
+  }
+
+  const { textSize } = props;
+  const fontSize = convertFontSizeToPx(textSize) || recommendedMinFontSizePx;
+
+  if (fontSize < recommendedMinFontSizePx) {
+    // eslint-disable-next-line
+    console.warn(
+      `prop \`textSize="${textSize}"\` is below the recommended minimum of ${recommendedMinFontSizePx}px`
+    ); // eslint-disable-line
+    return true;
+  }
+  return false;
+};
+
+const bgImageStyleWarning = component => {
+  const { props } = component;
+  const { bgLighten, bgDarken, bgImage, bgImageStyle, bgGradient } = props;
+
+  if (!bgImageStyle) {
+    return false;
+  }
+
+  if (bgImageStyle && (bgLighten || bgDarken || bgImage || bgGradient)) {
+    // eslint-disable-next-line
+    console.warn(
+      `The backgroundImage property has been set directly as \`bgImageStyle="${bgImageStyle}"\`.
+       Because bgImageStyle sets the backgroundImage to a string, the following 
+       properties which modify backgroundImage will not be applied:
+       ${bgLighten ? `bgLighten={${bgLighten}}` : ''}
+       ${bgDarken ? `bgDarken={${bgDarken}}` : ''}
+       ${bgImage ? `bgImage={${bgImage}}}` : ''}
+       ${bgImage ? `bgImage={${bgGradient}}}` : ''}`
+    );
+    return true;
+  }
+  return false;
+};
+
+const styleWarnings = [bgImageStyleWarning, fontSizeWarning];
+
+function checkWarnings(component, warnings = styleWarnings) {
+  if (!component.warnings) {
+    component.warnings = warnings;
+  }
+  if (component.warnings.length) {
+    component.warnings = component.warnings.filter(w => !w(component));
+  }
+}
+
+export default checkWarnings;


Test patch:
diff --git a/src/utils/base.test.js b/src/utils/base.test.js
index fb16f10..a98bca4 100644
--- a/src/utils/base.test.js
+++ b/src/utils/base.test.js
@@ -153,7 +153,9 @@ describe('getStyles', () => {
   describe('bgGradient', () => {
     it('should assign bgGradient value to backgroundImage', () => {
       const bgGradientValue = 'radial-gradient(red blue green)';
+      const bgImageValue = 'https://sameimage.com/url....';
       _this.props.bgGradient = bgGradientValue;
+      _this.props.bgImage = bgImageValue;
       const styles = generateStyles();
       expect(styles.backgroundImage).toEqual(bgGradientValue);
     });
@@ -167,52 +169,72 @@ describe('getStyles', () => {
       expect(styles.backgroundImage).toEqual(`url(${bgImageValue})`);
     });
   });
+});
 
-  describe('bgImage additional values', () => {
-    beforeEach(() => {
-      _this.props.bgImage = 'https://sameimage.com/url....';
-    });
-    it('should assign bgDarken value to backgroundImage opacity', () => {
-      const bgDarkenValue = 0.5;
-      const backgroundImageValue = `linear-gradient( rgba(0, 0, 0, ${bgDarkenValue}), rgba(0, 0, 0, ${bgDarkenValue}) ), url(${
-        _this.props.bgImage
-      })`;
-      _this.props.bgDarken = bgDarkenValue;
-      const styles = generateStyles();
-      expect(styles.backgroundImage).toEqual(backgroundImageValue);
-    });
+describe('bgImage additional values', () => {
+  let generateStyles;
+  let _this;
+  beforeEach(() => {
+    _this = {
+      props: { bgImage: 'https://sameimage.com/url....' },
+      context: {
+        styles: {
+          colors: { primary: 'pink' },
+          fonts: { primary: 'Helvetica' }
+        },
+        store: {
+          getState: () => ({
+            style: {
+              globalStyleSet: []
+            }
+          })
+        }
+      }
+    };
+    generateStyles = getStyles.bind(_this);
+  });
 
-    it('should assign bgSize value to backgroundSize', () => {
-      const bgSizeValue = 'auto';
-      _this.props.bgSize = bgSizeValue;
-      const styles = generateStyles();
-      expect(styles.backgroundSize).toEqual(bgSizeValue);
-    });
-    /* eslint-disable-next-line quotes */
-    it("should assign 'cover' to backgroundSize", () => {
-      const styles = generateStyles();
-      expect(styles.backgroundSize).toEqual('cover');
-    });
+  it('should assign bgDarken value to backgroundImage opacity', () => {
+    const bgDarkenValue = 0.5;
+    const backgroundImageValue = `linear-gradient( rgba(0, 0, 0, ${bgDarkenValue}), rgba(0, 0, 0, ${bgDarkenValue}) ), url(${
+      _this.props.bgImage
+    })`;
+    _this.props.bgDarken = bgDarkenValue;
+    const styles = generateStyles();
+    expect(styles.backgroundImage).toEqual(backgroundImageValue);
+  });
 
-    it('should assign bgPosition value to backgroundSize', () => {
-      const bgPositionValue = 'auto';
-      _this.props.bgPosition = bgPositionValue;
-      const styles = generateStyles();
-      expect(styles.backgroundPosition).toEqual(bgPositionValue);
-    });
+  it('should assign bgSize value to backgroundSize', () => {
+    const bgSizeValue = 'auto';
+    _this.props.bgSize = bgSizeValue;
+    const styles = generateStyles();
+    expect(styles.backgroundSize).toEqual(bgSizeValue);
+  });
 
-    /* eslint-disable-next-line quotes */
-    it("should assign 'center center' to backgroundSize", () => {
-      const styles = generateStyles();
-      expect(styles.backgroundPosition).toEqual('center center');
-    });
+  /* eslint-disable-next-line quotes */
+  it("should assign 'cover' to backgroundSize if a bgSize is not provided", () => {
+    const styles = generateStyles();
+    expect(styles.backgroundSize).toEqual('cover');
+  });
 
-    it('should assign bgRepeat value to backgroundSize', () => {
-      const bgRepeatValue = 'repeat';
-      _this.props.bgRepeat = bgRepeatValue;
-      const styles = generateStyles();
-      expect(styles.backgroundRepeat).toEqual(bgRepeatValue);
-    });
+  it('should assign bgPosition value to backgroundSize', () => {
+    const bgPositionValue = 'auto';
+    _this.props.bgPosition = bgPositionValue;
+    const styles = generateStyles();
+    expect(styles.backgroundPosition).toEqual(bgPositionValue);
+  });
+
+  /* eslint-disable-next-line quotes */
+  it("should assign 'center center' to backgroundSize if a bgPosition is not provided", () => {
+    const styles = generateStyles();
+    expect(styles.backgroundPosition).toEqual('center center');
+  });
+
+  it('should assign bgRepeat value to backgroundSize', () => {
+    const bgRepeatValue = 'repeat';
+    _this.props.bgRepeat = bgRepeatValue;
+    const styles = generateStyles();
+    expect(styles.backgroundRepeat).toEqual(bgRepeatValue);
   });
 
   describe('overflow', () => {