From 283f814e65aa525b1dfd48b7e3a9a2dd1b7bf974 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Louv-Jansen?= Date: Fri, 16 Feb 2018 02:37:32 +0100 Subject: [PATCH] Address feedback --- src-docs/src/views/delay_hide/delay_hide.js | 21 ++++---- .../views/delay_hide/delay_hide_example.js | 2 +- src/components/delay_hide/delay_hide.js | 32 ++++++----- src/components/delay_hide/delay_hide.test.js | 53 +++++++++++++------ 4 files changed, 67 insertions(+), 41 deletions(-) diff --git a/src-docs/src/views/delay_hide/delay_hide.js b/src-docs/src/views/delay_hide/delay_hide.js index 5da006f32b3c..ecb3b5546548 100644 --- 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 ( -
+
Hello world
} + render={() => } /> -
+ ); } } diff --git a/src-docs/src/views/delay_hide/delay_hide_example.js b/src-docs/src/views/delay_hide/delay_hide_example.js index 071574de7636..32ffe87c8e4c 100644 --- 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: (

- DelayHide is a component for conditionally toggling + EuiDelayHide 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 index 1f2b5b978cf9..52aa8cce1a30 100644 --- 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 index 069ca73b0dee..778b6ef1ee2c 100644 --- 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( -

Hello World
} /> +
Hello World
} + /> ); - wrapper.setProps({ hide: true }); }); test('it should be visible initially', async () => { + wrapper.setProps({ hide: true }); expect(wrapper.html()).toEqual('
Hello World
'); }); - 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('
Hello World
'); + }); + + 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('
Hello World
'); }); - 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('
Hello World
'); + + wrapper.setProps({ hide: true }); expect(wrapper.html()).toEqual(null); }); }); @@ -45,17 +67,14 @@ describe('when EuiDelayHide is hidden initially', () => { expect(wrapper.html()).toEqual('
Hello World
'); }); - 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('
Hello World
'); - wrapper.setProps({ hide: true }); - jest.advanceTimersByTime(500); + jest.advanceTimersByTime(900); expect(wrapper.html()).toEqual('
Hello World
'); - 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('
Hello World
'); }); - test('it should be visible after 1500ms', () => { - jest.advanceTimersByTime(1500); + test('it should be visible after 1900ms', () => { + jest.advanceTimersByTime(1900); expect(wrapper.html()).toEqual('
Hello World
'); }); - 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); }); });