Address feedback

elastic · Feb 16, 2018 · 283f814 · 283f814
1 parent ffe0fb7
commit 283f814
Show file tree

Hide file tree

Showing 4 changed files with 67 additions and 41 deletions.
diff --git a/src-docs/src/views/delay_hide/delay_hide.js b/src-docs/src/views/delay_hide/delay_hide.js
@@ -1,9 +1,12 @@
-import React, { Component } from 'react';
-import { EuiDelayHide } from '../../../../src/components/delay_hide';
-import { EuiFlexItem } from '../../../../src/components/flex/flex_item';
-import { EuiCheckbox } from '../../../../src/components/form/checkbox/checkbox';
-import { EuiFormRow } from '../../../../src/components/form/form_row/form_row';
-import { EuiFieldNumber } from '../../../../src/components/form/field_number';
+import React, { Component, Fragment } from 'react';
+import {
+  EuiDelayHide,
+  EuiFlexItem,
+  EuiCheckbox,
+  EuiFormRow,
+  EuiFieldNumber,
+  EuiLoadingSpinner
+} from '../../../../src/components';
 
 export default class extends Component {
   state = {
@@ -21,7 +24,7 @@ export default class extends Component {
 
   render() {
     return (
-      <div>
+      <Fragment>
         <EuiFlexItem>
           <EuiFormRow>
             <EuiCheckbox
@@ -42,11 +45,11 @@ export default class extends Component {
             <EuiDelayHide
               hide={this.state.hide}
               minimumDuration={this.state.minimumDuration}
-              render={() => <div>Hello world</div>}
+              render={() => <EuiLoadingSpinner size="m"/>}
             />
           </EuiFormRow>
         </EuiFlexItem>
-      </div>
+      </Fragment>
     );
   }
 }
diff --git a/src-docs/src/views/delay_hide/delay_hide_example.js b/src-docs/src/views/delay_hide/delay_hide_example.js
@@ -24,7 +24,7 @@ export const DelayHideExample = {
       ],
       text: (
         <p>
-          <EuiCode>DelayHide</EuiCode> is a component for conditionally toggling
+          <EuiCode>EuiDelayHide</EuiCode> is a component for conditionally toggling
           the visibility of a child component. It will ensure that the child is
           visible for at least 1000ms (default). This avoids UI glitches that
           are common with loading spinners and other elements that are rendered

diff --git a/src/components/delay_hide/delay_hide.js b/src/components/delay_hide/delay_hide.js
@@ -8,6 +8,11 @@ export class EuiDelayHide extends Component {
     render: PropTypes.func.isRequired
   };
 
+  static defaultProps = {
+    hide: false,
+    minimumDuration: 1000
+  };
+
   constructor(props) {
     super(props);
 
@@ -16,35 +21,34 @@ export class EuiDelayHide extends Component {
     };
 
     this.lastRenderedTime = this.props.hide ? 0 : Date.now();
-    this.shouldRender = false;
   }
 
-  componentWillReceiveProps({ hide, minimumDuration = 1000 }) {
+  getTimeRemaining(minimumDuration) {
+    const visibleDuration = Date.now() - this.lastRenderedTime;
+    return minimumDuration - visibleDuration;
+  }
+
+  componentWillReceiveProps(nextProps) {
     clearTimeout(this.timeout);
+    const timeRemaining = this.getTimeRemaining(nextProps.minimumDuration);
 
-    const visibleDuration = Date.now() - this.lastRenderedTime;
-    const timeRemaining = minimumDuration - visibleDuration;
-    if (hide && timeRemaining > 0) {
-      this.shouldRender = false;
+    if (nextProps.hide && timeRemaining > 0) {
       this.setStateDelayed(timeRemaining);
     } else {
-      this.shouldRender = true;
-      this.lastRenderedTime = Date.now();
-      this.setState({ hide });
+      if (this.state.hide && !nextProps.hide) {
+        this.lastRenderedTime = Date.now();
+      }
+
+      this.setState({ hide: nextProps.hide });
     }
   }
 
   setStateDelayed = timeRemaining => {
     this.timeout = setTimeout(() => {
-      this.shouldRender = true;
       this.setState({ hide: true });
     }, timeRemaining);
   };
 
-  shouldComponentUpdate() {
-    return this.shouldRender;
-  }
-
   componentWillUnmount() {
     clearTimeout(this.timeout);
   }

diff --git a/src/components/delay_hide/delay_hide.test.js b/src/components/delay_hide/delay_hide.test.js
@@ -2,27 +2,49 @@ import React from 'react';
 import { mount } from 'enzyme';
 import { EuiDelayHide } from './index';
 
-describe('when EuiDelayHide is visible initially and is changed to hidden', () => {
+describe('when EuiDelayHide is visible initially', () => {
   let wrapper;
   beforeEach(() => {
     jest.useFakeTimers();
     wrapper = mount(
-      <EuiDelayHide hide={false} render={() => <div>Hello World</div>} />
+      <EuiDelayHide
+        hide={false}
+        render={() => <div>Hello World</div>}
+      />
     );
-    wrapper.setProps({ hide: true });
   });
 
   test('it should be visible initially', async () => {
+    wrapper.setProps({ hide: true });
     expect(wrapper.html()).toEqual('<div>Hello World</div>');
   });
 
-  test('it should be visible after 500ms', () => {
-    jest.advanceTimersByTime(500);
+  test('it should be visible after 900ms', () => {
+    wrapper.setProps({ hide: true });
+    jest.advanceTimersByTime(900);
+    expect(wrapper.html()).toEqual('<div>Hello World</div>');
+  });
+
+  test('it should be hidden after 1100ms', () => {
+    wrapper.setProps({ hide: true });
+    jest.advanceTimersByTime(1100);
+    expect(wrapper.html()).toEqual(null);
+  });
+
+  test('it should be visible after 1100ms regardless of prop changes in-between', () => {
+    wrapper.setProps({ hide: true });
+    wrapper.setProps({ hide: false });
+    jest.advanceTimersByTime(1100);
     expect(wrapper.html()).toEqual('<div>Hello World</div>');
   });
 
-  test('it should be hidden after 1500ms', () => {
-    jest.advanceTimersByTime(1500);
+  test('it should hide immediately after prop change, if it has been displayed for 1100ms', () => {
+    const currentTime = Date.now();
+    jest.advanceTimersByTime(1100);
+    jest.spyOn(Date, 'now').mockReturnValue(currentTime + 1100);
+    expect(wrapper.html()).toEqual('<div>Hello World</div>');
+
+    wrapper.setProps({ hide: true });
     expect(wrapper.html()).toEqual(null);
   });
 });
@@ -45,17 +67,14 @@ describe('when EuiDelayHide is hidden initially', () => {
     expect(wrapper.html()).toEqual('<div>Hello World</div>');
   });
 
-  test('it should become visible immediately after prop change but not become hidden until after 1000ms', async () => {
+  test('it should be visible for at least 1100ms before hiding', async () => {
     wrapper.setProps({ hide: false });
-    expect(wrapper.html()).toEqual('<div>Hello World</div>');
-
     wrapper.setProps({ hide: true });
-    jest.advanceTimersByTime(500);
+    jest.advanceTimersByTime(900);
 
     expect(wrapper.html()).toEqual('<div>Hello World</div>');
 
-    jest.advanceTimersByTime(1000);
-
+    jest.advanceTimersByTime(200);
     expect(wrapper.html()).toEqual(null);
   });
 });
@@ -78,13 +97,13 @@ describe('when EuiDelayHide is visible initially and has a minimumDuration of 20
     expect(wrapper.html()).toEqual('<div>Hello World</div>');
   });
 
-  test('it should be visible after 1500ms', () => {
-    jest.advanceTimersByTime(1500);
+  test('it should be visible after 1900ms', () => {
+    jest.advanceTimersByTime(1900);
     expect(wrapper.html()).toEqual('<div>Hello World</div>');
   });
 
-  test('it should be hidden after 2500ms', () => {
-    jest.advanceTimersByTime(2500);
+  test('it should be hidden after 2100ms', () => {
+    jest.advanceTimersByTime(2100);
     expect(wrapper.html()).toEqual(null);
   });
 });