Enhancement rename action props with on prefix, and action methods …

…with `handle` prefix
silverstripe · Sep 19, 2017 · f607547 · f607547
1 parent d477850
commit f607547
Show file tree

Hide file tree

Showing 28 changed files with 295 additions and 328 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -45,6 +45,9 @@ module.exports = {
         "vars": "local"
       }
     ],
+      "react/no-danger": [
+        "error"
+      ],
     "react/forbid-prop-types": [
       "off"
     ],
@@ -60,9 +63,6 @@ module.exports = {
     "no-useless-escape": [
       "off"
     ],
-    "react/no-danger": [
-      "error"
-    ],
   }),
   "settings": {
     "import/extensions": [

diff --git a/client/src/components/Breadcrumb/Breadcrumb.js b/client/src/components/Breadcrumb/Breadcrumb.js
@@ -1,4 +1,4 @@
-import React, { Component } from 'react';
+import React, { Component, PropTypes } from 'react';
 import { connect } from 'react-redux';
 
 class Breadcrumb extends Component {
@@ -44,7 +44,7 @@ class Breadcrumb extends Component {
               className={iconClassNames.join(' ')}
               role="button"
               tabIndex={0}
-              onClick={crumb.icon.action}
+              onClick={crumb.icon.onClick}
             />
           )}
         </h2>
@@ -67,7 +67,15 @@ class Breadcrumb extends Component {
 }
 
 Breadcrumb.propTypes = {
-  crumbs: React.PropTypes.array,
+  crumbs: PropTypes.arrayOf(PropTypes.shape({
+    onClick: PropTypes.func,
+    text: PropTypes.string,
+    icon: PropTypes.shape({
+      className: PropTypes.string,
+      onClick: PropTypes.func,
+      action: (props) => { if (props.action) { throw new Error('action: no longer used'); } },
+    })
+  })),
 };
 
 function mapStateToProps(state) {

diff --git a/client/src/components/Breadcrumb/tests/breadcrumb-test.js b/client/src/components/Breadcrumb/tests/breadcrumb-test.js
@@ -25,7 +25,7 @@ describe('BreadcrumbsComponent', () => {
           href: 'href3',
           icon: {
             className: 'breadcrumb3icon',
-            action: jest.genMockFunction(),
+            onClick: jest.fn(),
           },
         },
       ];

diff --git a/client/src/components/CheckboxField/CheckboxField.js b/client/src/components/CheckboxField/CheckboxField.js
@@ -1,15 +1,13 @@
-import React, { Component } from 'react';
+import React from 'react';
 import OptionField from '../OptionsetField/OptionField';
 import fieldHolder from 'components/FieldHolder/FieldHolder';
 
-class CheckboxField extends Component {
-  render() {
-    // Build standard checkbox with fieldholder
-    const FieldHolder = fieldHolder(OptionField);
+const CheckboxField = () => {
+  // Build standard checkbox with fieldholder
+  const FieldHolder = fieldHolder(OptionField);
 
-    // set to not show field holder labels, as checkbox already generates a label
-    return <FieldHolder {...this.props} type="checkbox" hideLabels />;
-  }
-}
+  // set to not show field holder labels, as checkbox already generates a label
+  return <FieldHolder {...this.props} type="checkbox" hideLabels />;
+};
 
 export default CheckboxField;
diff --git a/client/src/components/CheckboxSetField/CheckboxSetField.js b/client/src/components/CheckboxSetField/CheckboxSetField.js
@@ -44,30 +44,6 @@ class CheckboxSetField extends Component {
     return [];
   }
 
-  /**
-   * Handler for sorting what the value of the field will be, this flows on from the
-   * OptionField (single checkbox) event handler and adding or removing the corresponding value the
-   * single checkbox represented to suit the multiple checkbox group.
-   *
-   * @param {Event} event
-   * @param {object} field
-   */
-  handleChange(event, field) {
-    if (typeof this.props.onChange === 'function') {
-      const oldValue = this.getValues();
-      const newValue = this.props.source
-        .filter((item, index) => {
-          if (this.getItemKey(item, index) === field.id) {
-            return field.value === 1;
-          }
-          return oldValue.indexOf(`${item.value}`) > -1;
-        })
-        .map((item) => `${item.value}`);
-
-      this.props.onChange(newValue);
-    }
-  }
-
   /**
    * Fetches properties for an item
    *
@@ -93,6 +69,30 @@ class CheckboxSetField extends Component {
     };
   }
 
+  /**
+   * Handler for sorting what the value of the field will be, this flows on from the
+   * OptionField (single checkbox) event handler and adding or removing the corresponding value the
+   * single checkbox represented to suit the multiple checkbox group.
+   *
+   * @param {Event} event
+   * @param {object} field
+   */
+  handleChange(event, field) {
+    if (typeof this.props.onChange === 'function') {
+      const oldValue = this.getValues();
+      const newValue = this.props.source
+        .filter((item, index) => {
+          if (this.getItemKey(item, index) === field.id) {
+            return field.value === 1;
+          }
+          return oldValue.indexOf(`${item.value}`) > -1;
+        })
+        .map((item) => `${item.value}`);
+
+      this.props.onChange(newValue);
+    }
+  }
+
   render() {
     if (!this.props.source) {
       return null;

diff --git a/client/src/components/FieldHolder/FieldHolder.js b/client/src/components/FieldHolder/FieldHolder.js
@@ -27,10 +27,36 @@ function fieldHolder(Field) {
       return message;
     }
 
+    /**
+     * Generates the properties for the field holder
+     *
+     * @returns {object}
+     */
+    getHolderProps() {
+      // The extraClass property is defined on both the holder and element
+      // for legacy reasons (same behaviour as PHP rendering)
+      const classNames = [
+        'field',
+        this.props.extraClass,
+      ];
+      if (this.props.readOnly) {
+        classNames.push('readonly');
+      }
+
+      return {
+        bsClass: this.props.bsClass,
+        bsSize: this.props.bsSize,
+        validationState: this.props.validationState,
+        className: classNames.join(' '),
+        controlId: this.props.id,
+        id: this.props.holderId,
+      };
+    }
+
     /**
      * Build description
      *
-     * @returns {Component}
+     * @returns {React}
      */
     renderDescription() {
       if (this.props.description === null) {
@@ -47,7 +73,7 @@ function fieldHolder(Field) {
     /**
      * Build a FormAlert
      *
-     * @returns {Component}
+     * @returns {React}
      */
     renderMessage() {
       const message = this.getMessage();
@@ -66,7 +92,7 @@ function fieldHolder(Field) {
     /**
      * Build title label
      *
-     * @returns {Component}
+     * @returns {React}
      */
     renderLeftTitle() {
       const labelText = this.props.leftTitle !== null
@@ -87,7 +113,7 @@ function fieldHolder(Field) {
     /**
      * Build title label
      *
-     * @returns {Component}
+     * @returns {React}
      */
     renderRightTitle() {
       if (!this.props.rightTitle || this.props.hideLabels) {
@@ -101,32 +127,6 @@ function fieldHolder(Field) {
       );
     }
 
-    /**
-     * Generates the properties for the field holder
-     *
-     * @returns {object}
-     */
-    getHolderProps() {
-      // The extraClass property is defined on both the holder and element
-      // for legacy reasons (same behaviour as PHP rendering)
-      const classNames = [
-        'field',
-        this.props.extraClass,
-      ];
-      if (this.props.readOnly) {
-        classNames.push('readonly');
-      }
-
-      return {
-        bsClass: this.props.bsClass,
-        bsSize: this.props.bsSize,
-        validationState: this.props.validationState,
-        className: classNames.join(' '),
-        controlId: this.props.id,
-        id: this.props.holderId,
-      };
-    }
-
     renderField() {
       const hasMessage = Boolean(this.getMessage());
       const props = {
@@ -137,7 +137,7 @@ function fieldHolder(Field) {
         ),
       };
 
-      const field = <Field { ...props} />;
+      const field = <Field {...props} />;
       const prefix = this.props.data.prefix;
       const suffix = this.props.data.suffix;
       if (!prefix && !suffix) {

diff --git a/client/src/components/Form/Form.js b/client/src/components/Form/Form.js
@@ -92,6 +92,7 @@ Form.propTypes = {
     method: PropTypes.string.isRequired,
   }),
   fields: PropTypes.array.isRequired,
+  // props is named `handleSubmit` as it is recieved from redux-form
   handleSubmit: PropTypes.func,
   mapActionsToComponents: PropTypes.func.isRequired,
   mapFieldsToComponents: PropTypes.func.isRequired,

diff --git a/client/src/components/FormAction/FormAction.js b/client/src/components/FormAction/FormAction.js
@@ -9,17 +9,6 @@ class FormAction extends Component {
     this.handleClick = this.handleClick.bind(this);
   }
 
-  render() {
-    const title = this.props.title;
-
-    return (
-      <button {...this.getButtonProps()}>
-        {this.getLoadingIcon()}
-        {castStringToElement('span', title, { className: 'btn__title' })}
-      </button>
-    );
-  }
-
   /**
    * Get props for the button
    *
@@ -71,17 +60,6 @@ class FormAction extends Component {
     return classnames(buttonClasses);
   }
 
-  /**
-   * @return {boolean}
-   */
-  isPrimary() {
-    const extraClasses = this.props.extraClass.split(' ');
-    return (
-      this.props.name === 'action_save' ||
-      extraClasses.find(className => className === 'ss-ui-action-constructive')
-    );
-  }
-
   /**
    * Gets the bootstrap classname for this action
    *
@@ -140,23 +118,46 @@ class FormAction extends Component {
     return null;
   }
 
+  /**
+   * @return {boolean}
+   */
+  isPrimary() {
+    const extraClasses = this.props.extraClass.split(' ');
+    return (
+      this.props.name === 'action_save' ||
+      extraClasses.find(className => className === 'ss-ui-action-constructive')
+    );
+  }
+
   /**
    * Event handler triggered when a user clicks the button.
    *
    * @param {Object} event
    * @return undefined
    */
   handleClick(event) {
-    if (typeof this.props.handleClick === 'function') {
-      this.props.handleClick(event, this.props.name || this.props.id);
+    if (typeof this.props.onClick === 'function') {
+      this.props.onClick(event, this.props.name || this.props.id);
     }
   }
+
+  render() {
+    const title = this.props.title;
+
+    return (
+      <button {...this.getButtonProps()}>
+        {this.getLoadingIcon()}
+        {castStringToElement('span', title, { className: 'btn__title' })}
+      </button>
+    );
+  }
 }
 
 FormAction.propTypes = {
   id: React.PropTypes.string,
   name: React.PropTypes.string,
-  handleClick: React.PropTypes.func,
+  onClick: React.PropTypes.func,
+  handleClick: () => { throw new Error('no longer used'); },
   title: React.PropTypes.string,
   type: React.PropTypes.string,
   loading: React.PropTypes.bool,

diff --git a/client/src/components/FormAlert/FormAlert.js b/client/src/components/FormAlert/FormAlert.js
@@ -17,17 +17,6 @@ class FormAlert extends Component {
     };
   }
 
-  /**
-   * Handler for when the message box is dismissed and hidden
-   */
-  handleDismiss() {
-    if (typeof this.props.onDismiss === 'function') {
-      this.props.onDismiss();
-    } else {
-      this.setState({ visible: false });
-    }
-  }
-
   /**
    * Determines the style for the alert box to be shown based on messageType or other property
    * by default use "danger".
@@ -71,6 +60,17 @@ class FormAlert extends Component {
     };
   }
 
+  /**
+   * Handler for when the message box is dismissed and hidden
+   */
+  handleDismiss() {
+    if (typeof this.props.onDismiss === 'function') {
+      this.props.onDismiss();
+    } else {
+      this.setState({ visible: false });
+    }
+  }
+
   render() {
     // @todo default this.props.visible as null
     if ((typeof this.props.visible !== 'boolean' && this.state.visible) || this.props.visible) {