Merge pull request #159 from paulkaplan/aggregate-color-updates

Only submit SVG changes when color picker closes

src/containers/fill-color-indicator.jsx

@@ -15,9 +15,22 @@ class FillColorIndicator extends React.Component {
         bindAll(this, [
             'handleChangeFillColor'
         ]);
+
+        // Flag to track whether an svg-update-worthy change has been made
+        this._hasChanged = false;
+    }
+    componentWillReceiveProps (newProps) {
+        const {fillColorModalVisible, onUpdateSvg} = this.props;
+        if (fillColorModalVisible && !newProps.fillColorModalVisible) {
+            // Submit the new SVG, which also stores a single undo/redo action.
+            if (this._hasChanged) onUpdateSvg();
+            this._hasChanged = false;
+        }
     }
     handleChangeFillColor (newColor) {
-        applyFillColorToSelection(newColor, this.props.onUpdateSvg);
+        // Apply color and update redux, but do not update svg until picker closes.
+        const isDifferent = applyFillColorToSelection(newColor);
+        this._hasChanged = this._hasChanged || isDifferent;
         this.props.onChangeFillColor(newColor);
     }
     render () {
@@ -51,6 +64,7 @@ const mapDispatchToProps = dispatch => ({
 FillColorIndicator.propTypes = {
     disabled: PropTypes.bool.isRequired,
     fillColor: PropTypes.string,
+    fillColorModalVisible: PropTypes.bool.isRequired,
     onChangeFillColor: PropTypes.func.isRequired,
     onUpdateSvg: PropTypes.func.isRequired
 };

src/containers/stroke-color-indicator.jsx

@@ -15,9 +15,22 @@ class StrokeColorIndicator extends React.Component {
         bindAll(this, [
             'handleChangeStrokeColor'
         ]);
+
+        // Flag to track whether an svg-update-worthy change has been made
+        this._hasChanged = false;
+    }
+    componentWillReceiveProps (newProps) {
+        const {strokeColorModalVisible, onUpdateSvg} = this.props;
+        if (strokeColorModalVisible && !newProps.strokeColorModalVisible) {
+            // Submit the new SVG, which also stores a single undo/redo action.
+            if (this._hasChanged) onUpdateSvg();
+            this._hasChanged = false;
+        }
     }
     handleChangeStrokeColor (newColor) {
-        applyStrokeColorToSelection(newColor, this.props.onUpdateSvg);
+        // Apply color and update redux, but do not update svg until picker closes.
+        const isDifferent = applyStrokeColorToSelection(newColor);
+        this._hasChanged = this._hasChanged || isDifferent;
         this.props.onChangeStrokeColor(newColor);
     }
     render () {
@@ -52,7 +65,8 @@ StrokeColorIndicator.propTypes = {
     disabled: PropTypes.bool.isRequired,
     onChangeStrokeColor: PropTypes.func.isRequired,
     onUpdateSvg: PropTypes.func.isRequired,
-    strokeColor: PropTypes.string
+    strokeColor: PropTypes.string,
+    strokeColorModalVisible: PropTypes.bool.isRequired
 };
 
 export default connect(

src/helper/style-path.js

@@ -8,9 +8,9 @@ const MIXED = 'scratch-paint/style-path/mixed';
 /**
  * Called when setting fill color
  * @param {string} colorString New color, css format
- * @param {!function} onUpdateSvg A callback to call when the image visibly changes
+ * @return {boolean} Whether the color application actually changed visibly.
  */
-const applyFillColorToSelection = function (colorString, onUpdateSvg) {
+const applyFillColorToSelection = function (colorString) {
     const items = getSelectedLeafItems();
     let changed = false;
     for (const item of items) {
@@ -45,9 +45,7 @@ const applyFillColorToSelection = function (colorString, onUpdateSvg) {
             }
         }
     }
-    if (changed) {
-        onUpdateSvg();
-    }
+    return changed;
 };
 
 const _strokeColorMatch = function (item, incomingColor) {
@@ -58,9 +56,9 @@ const _strokeColorMatch = function (item, incomingColor) {
 /**
  * Called when setting stroke color
  * @param {string} colorString New color, css format
- * @param {!function} onUpdateSvg A callback to call when the image visibly changes
+ * @return {boolean} Whether the color application actually changed visibly.
  */
-const applyStrokeColorToSelection = function (colorString, onUpdateSvg) {
+const applyStrokeColorToSelection = function (colorString) {
     const items = getSelectedLeafItems();
     let changed = false;
     for (const item of items) {
@@ -94,9 +92,7 @@ const applyStrokeColorToSelection = function (colorString, onUpdateSvg) {
             item.strokeColor = colorString;
         }
     }
-    if (changed) {
-        onUpdateSvg();
-    }
+    return changed;
 };
 
 /**
@@ -132,11 +128,11 @@ const getColorsFromSelection = function (selectedItems) {
     let selectionStrokeColorString;
     let selectionStrokeWidth;
     let firstChild = true;
-    
+
     for (const item of selectedItems) {
         let itemFillColorString;
         let itemStrokeColorString;
-        
+
         // handle pgTextItems differently by going through their children
         if (isPGTextItem(item)) {
             for (const child of item.children) {