Merge #279 #280

279: Fix URL for new extension button r=magopian a=rehandalal r? 280: Improve IdenticonField and fix warnings r=magopian a=rehandalal Sorry @magopian... After reviewing your PR I realized that `IdenticonField` needed a bit of a refactor and ended up fixing all the things. Fixes #89. Fixes #202. r? Co-authored-by: Rehan Dalal <[email protected]>
mozilla · Jul 9, 2018 · bfcb319 · bfcb319
3 parents 95699e2 + b698b88 + 06eaea5
commit bfcb319
Show file tree

Hide file tree

Showing 9 changed files with 93 additions and 107 deletions.
diff --git a/src/components/data/QueryRevision.js b/src/components/data/QueryRevision.js
@@ -13,7 +13,7 @@ import { fetchRevision } from 'console/state/revisions/actions';
 export default class QueryRevision extends React.PureComponent {
   static propTypes = {
     fetchRevision: PropTypes.func.isRequired,
-    pk: PropTypes.string.isRequired,
+    pk: PropTypes.number.isRequired,
   };
 
   componentWillMount() {

diff --git a/src/components/extensions/ListingActionBar.js b/src/components/extensions/ListingActionBar.js
@@ -49,7 +49,7 @@ export default class ListingActionBar extends React.PureComponent {
           />
         </Col>
         <Col span={8} className="righted">
-          <Link to={reverse('recipes.new')}>
+          <Link to={reverse('extensions.new')}>
             <Button type="primary" icon="plus">
               New Extension
             </Button>

diff --git a/src/components/forms/IdenticonField.js b/src/components/forms/IdenticonField.js
@@ -10,13 +10,12 @@ import ShieldIdenticon from 'console/components/common/ShieldIdenticon';
 export default class IdenticonField extends React.PureComponent {
   static propTypes = {
     disabled: PropTypes.bool,
-    onChange: PropTypes.func.isRequired,
+    onChange: PropTypes.func,
     value: PropTypes.any,
   };
 
   static defaultProps = {
     disabled: false,
-    value: null,
   };
 
   static generateSeed() {
@@ -34,54 +33,65 @@ export default class IdenticonField extends React.PureComponent {
     };
   }
 
-  componentWillReceiveProps({ value }) {
-    if (this.state.index === -1 && value) {
-      this.setState({
-        index: 0,
-        history: fromJS([value]),
+  componentDidUpdate(prevProps) {
+    const { value } = this.props;
+    if (value && value !== prevProps.value) {
+      this.setState(({ history, index }) => {
+        const newIndex = history.includes(value) ? history.indexOf(value) : history.size;
+        const newHistory = newIndex === history.size ? history.push(value) : history;
+
+        return {
+          index: newIndex,
+          history: newHistory,
+        };
       });
     }
   }
 
-  handleChange(direction) {
-    let next;
+  navigateHistory(step) {
+    const { onChange } = this.props;
     const { index, history } = this.state;
-    const newIndex = index + direction;
-
-    if (newIndex < 0) {
-      return;
-    }
-
-    next = history.get(newIndex);
+    const newIndex = index + step;
+    let next;
 
-    let newHistory = history;
-    if (!next) {
-      next = IdenticonField.generateSeed();
+    if (newIndex >= 0) {
+      next = history.get(newIndex, IdenticonField.generateSeed());
 
-      // Ensure duplicate entries are not saved in history.
-      if (newHistory.indexOf(next) === -1) {
-        newHistory = newHistory.push(next);
+      if (onChange) {
+        onChange(next);
       }
     }
-
-    this.setState({
-      index: newIndex,
-      history: newHistory,
-    });
-    this.props.onChange(next);
   }
 
   handlePrev() {
-    this.handleChange(-1);
+    this.navigateHistory(-1);
   }
 
   handleNext() {
-    this.handleChange(1);
+    this.navigateHistory(1);
   }
 
-  render() {
+  renderIdenticon() {
     const { value } = this.props;
 
+    if (!value) {
+      return null;
+    }
+
+    return (
+      <Popover
+        mouseEnterDelay={0.75}
+        content={<ShieldIdenticon seed={value} size={256} />}
+        placement="right"
+      >
+        <div className="shield-container">
+          <ShieldIdenticon seed={value} />
+        </div>
+      </Popover>
+    );
+  }
+
+  render() {
     return (
       <div className="identicon-field">
         <Button
@@ -94,15 +104,7 @@ export default class IdenticonField extends React.PureComponent {
           <Icon type="left" />
         </Button>
 
-        <Popover
-          mouseEnterDelay={0.75}
-          content={<ShieldIdenticon seed={value} size={256} />}
-          placement="right"
-        >
-          <div className="shield-container">
-            <ShieldIdenticon seed={value} />
-          </div>
-        </Popover>
+        {this.renderIdenticon()}
 
         <Button
           className="btn-next"

diff --git a/src/components/pages/recipes/CreateRecipePage.js b/src/components/pages/recipes/CreateRecipePage.js
@@ -22,7 +22,7 @@ import { reverse } from 'console/urls';
 export default class CreateRecipePage extends React.PureComponent {
   static propTypes = {
     createRecipe: PropTypes.func.isRequired,
-    push: PropTypes.object.isRequired,
+    push: PropTypes.func.isRequired,
   };
 
   onFormFailure(err) {

diff --git a/src/components/recipes/RecipeForm.js b/src/components/recipes/RecipeForm.js
@@ -75,8 +75,12 @@ export default class RecipeForm extends React.PureComponent {
     'opt-out-study': OptOutStudyFields,
   };
 
-  componentDidMount() {
-    this.defaultIdenticonSeed = IdenticonField.generateSeed();
+  constructor(props) {
+    super(props);
+
+    this.state = {
+      defaultIdenticonSeed: IdenticonField.generateSeed(),
+    };
   }
 
   componentDidUpdate(prevProps) {
@@ -109,10 +113,11 @@ export default class RecipeForm extends React.PureComponent {
 
   render() {
     const { isCreationForm, isLoading, onSubmit, recipe } = this.props;
+    const { defaultIdenticonSeed } = this.state;
 
     // If creating, the 'default' seed is randomly generated. We store it in memory
     // to prevent the form from generating a new identicon on each render.
-    const identiconSeed = isCreationForm ? this.defaultIdenticonSeed : null;
+    const identiconSeed = isCreationForm ? defaultIdenticonSeed : null;
 
     return (
       <Form onSubmit={onSubmit} className="recipe-form">

diff --git a/src/tests/components/data/QueryRevision.test.js b/src/tests/components/data/QueryRevision.test.js
@@ -6,7 +6,7 @@ const { WrappedComponent: QueryRevision } = TestComponent;
 describe('<QueryRevision>', () => {
   const props = {
     fetchRevision: () => {},
-    pk: '1',
+    pk: 1,
   };
 
   it('should work', () => {
@@ -41,16 +41,16 @@ describe('<QueryRevision>', () => {
     );
     expect(callCount).toBe(1);
 
-    wrapper.setProps({ pk: '2' });
+    wrapper.setProps({ pk: 2 });
     expect(callCount).toBe(2);
 
     wrapper.setProps({ irrelevant: true });
     expect(callCount).toBe(2);
 
-    wrapper.setProps({ pk: '2' });
+    wrapper.setProps({ pk: 2 });
     expect(callCount).toBe(2);
 
-    wrapper.setProps({ pk: '3' });
+    wrapper.setProps({ pk: 3 });
     expect(callCount).toBe(3);
   });
 

diff --git a/src/tests/components/forms/IdenticonField.test.js b/src/tests/components/forms/IdenticonField.test.js
@@ -1,16 +1,24 @@
 import IdenticonField from 'console/components/forms/IdenticonField';
 
-describe('<IdenticonField>', () => {
-  const props = {
-    disabled: false,
-    onChange: () => {},
-    value: 'test',
+const defaultProps = {
+  disabled: false,
+  value: 'test',
+};
+
+function createIndenticonFieldWrapper(props) {
+  let wrapper;
+  const fieldProps = {
+    ...defaultProps,
+    onChange: v => wrapper.setProps({ value: v }),
+    ...props,
   };
+  wrapper = mount(<IdenticonField {...fieldProps} />);
+  return wrapper;
+}
 
+describe('<IdenticonField>', () => {
   it('should work', () => {
-    const wrapper = () => mount(<IdenticonField {...props} />);
-
-    expect(wrapper).not.toThrow();
+    expect(createIndenticonFieldWrapper).not.toThrow();
   });
 
   it('should use the v1 shield generation', () => {
@@ -19,87 +27,57 @@ describe('<IdenticonField>', () => {
 
   describe('selection buttons', () => {
     it('should generate a new item if at the end of the history', () => {
-      let { value } = props;
-      const wrapper = mount(
-        <IdenticonField
-          {...props}
-          onChange={val => {
-            value = val;
-          }}
-        />,
-      );
+      const wrapper = createIndenticonFieldWrapper();
       const next = wrapper.find('button.btn-next');
-
       next.simulate('click');
-
-      expect(value).not.toBe(props.value);
+      expect(wrapper.prop('value')).not.toBe(defaultProps.value);
     });
 
     it('should track a history of viewed icons', () => {
-      let { value } = props;
-      const wrapper = mount(
-        <IdenticonField
-          {...props}
-          onChange={val => {
-            value = val;
-          }}
-        />,
-      );
+      const wrapper = createIndenticonFieldWrapper();
       const next = wrapper.find('button.btn-next');
 
       expect(wrapper.state().history.size).toBe(1);
-      expect(value).toBe(props.value);
+      expect(wrapper.prop('value')).toBe(defaultProps.value);
 
       next.simulate('click');
 
       expect(wrapper.state().history.size).toBe(2);
-      expect(value).not.toBe(props.value);
+      expect(wrapper.prop('value')).not.toBe(defaultProps.value);
     });
 
     it('should go back in history if possible', () => {
-      let { value } = props;
-      const wrapper = mount(
-        <IdenticonField
-          {...props}
-          onChange={val => {
-            value = val;
-          }}
-        />,
-      );
+      const wrapper = createIndenticonFieldWrapper();
+
       let prev = wrapper.find('button.btn-prev');
       // Disabled at first until we move forward in history
-      expect(prev.props().disabled).toBe(true);
-      const originalValue = value;
+      expect(prev.prop('disabled')).toBe(true);
 
       const next = wrapper.find('button.btn-next');
       next.simulate('click');
 
       prev = wrapper.find('button.btn-prev');
-      expect(prev.props().disabled).toBe(false);
+      expect(prev.prop('disabled')).toBe(false);
+
+      prev.simulate('click');
+      expect(wrapper.prop('value')).toBe(defaultProps.value);
 
       prev.simulate('click');
-      expect(value).toBe(originalValue);
+      expect(prev.prop('disabled')).toBe(false);
+      expect(wrapper.prop('value')).toBe(defaultProps.value);
     });
 
     it('should recall icons from history if moving forward', () => {
-      let { value } = props;
-      const wrapper = mount(
-        <IdenticonField
-          {...props}
-          onChange={val => {
-            value = val;
-          }}
-        />,
-      );
+      const wrapper = createIndenticonFieldWrapper();
       const prev = wrapper.find('button.btn-prev');
       const next = wrapper.find('button.btn-next');
 
       next.simulate('click');
-      const originalValue = value;
+      const originalValue = wrapper.prop('value');
       next.simulate('click');
 
       prev.simulate('click');
-      expect(value).toBe(originalValue);
+      expect(wrapper.prop('value')).toBe(originalValue);
     });
   });
 });
diff --git a/src/tests/components/pages/MissingPage.test.js b/src/tests/components/pages/MissingPage.test.js
@@ -4,8 +4,7 @@ const { WrappedComponent: MissingPage } = TestComponent;
 
 describe('<MissingPage>', () => {
   it('should work', () => {
-    const wrapper = () => shallow(<MissingPage />);
-
+    const wrapper = () => shallow(<MissingPage pathname="/oops/" />);
     expect(wrapper).not.toThrow();
   });
 });
diff --git a/src/tests/components/tables/DataList.test.js b/src/tests/components/tables/DataList.test.js
@@ -9,7 +9,9 @@ describe('<DataList>', () => {
     columns: new List(),
     dataSource: [],
     getCurrentUrl: () => {},
+    getUrlFromRecord: () => {},
     history: {},
+    openNewWindow: () => {},
     ordering: 'surprise-me',
     onRowClick: () => {},
     push: () => {},