From f085cbaa8131509b49bd60846a59808559df6119 Mon Sep 17 00:00:00 2001 From: Martin Bielik Date: Fri, 12 May 2017 12:13:09 +0200 Subject: [PATCH 1/3] closing animation for controlled menus --- src/MenuContext.js | 47 ++++++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/src/MenuContext.js b/src/MenuContext.js index 89810db..39370da 100644 --- a/src/MenuContext.js +++ b/src/MenuContext.js @@ -48,23 +48,32 @@ export default class MenuContext extends Component { return Promise.resolve(); } - closeMenu() { + closeMenu() { // has no effect on controlled menus debug('close menu'); + this._menuRegistry.getAll() + .filter(menu => menu.instance._getOpened()) + .forEach(menu => menu.instance._setOpened(false)); + this._notify(); + } + + _invalidateTriggerLayouts() { + // invalidate layouts for closed menus, + // both controlled and uncontrolled menus + this._menuRegistry.getAll() + .filter(menu => !menu.instance._isOpen()) + .forEach(menu => { + this._menuRegistry.updateLayoutInfo(menu.name, { triggerLayout: undefined }); + }); + } + + _beforeClose(menu) { + debug('before close', menu.name); const hideMenu = (this.refs.menuOptions && this.refs.menuOptions.close && this.refs.menuOptions.close()) || Promise.resolve(); const hideBackdrop = this.refs.backdrop && this.refs.backdrop.close(); - const closePromise = Promise.all([hideMenu, hideBackdrop]); - return closePromise.then(() => { - this._menuRegistry.getAll().forEach(menu => { - if (menu.instance._getOpened()) { - menu.instance._setOpened(false); - // invalidate trigger layout - this._menuRegistry.updateLayoutInfo(menu.name, { triggerLayout: undefined }); - } - }); - this._notify(); - }).catch(console.error); + this._invalidateTriggerLayouts(); + return Promise.all([hideMenu, hideBackdrop]); } toggleMenu(name) { @@ -91,15 +100,21 @@ export default class MenuContext extends Component { } debug('notify: next menu:', next.name, ' prev menu:', prev.name); let afterSetState = undefined; + let beforeSetState = Promise.resolve; if (prev.name !== next.name) { - prev.instance && prev.instance.props.onClose(); - if (next.name) { + if (prev !== NULL && !prev.instance._isOpen()) { + beforeSetState = () => this._beforeClose(prev) + .then(() => prev.instance.props.onClose()); + } + if (next !== NULL) { next.instance.props.onOpen(); afterSetState = () => this._initOpen(next); } } - this.setState({ openedMenu: this.openedMenu }, afterSetState); - debug('notify ended'); + beforeSetState().then(() => { + this.setState({ openedMenu: this.openedMenu }, afterSetState); + debug('notify ended'); + }); } /** From de0dfca60666384b5d9cc233a06ce41f982f631b Mon Sep 17 00:00:00 2001 From: Martin Bielik Date: Fri, 12 May 2017 14:12:22 +0200 Subject: [PATCH 2/3] fixed tests, updated dev dependencies --- __tests__/MenuContext-test.js | 111 ++++++++++++++++++---------------- package.json | 30 ++++----- src/MenuContext.js | 17 +++--- 3 files changed, 83 insertions(+), 75 deletions(-) diff --git a/__tests__/MenuContext-test.js b/__tests__/MenuContext-test.js index 2bdf13f..bd0f4c9 100644 --- a/__tests__/MenuContext-test.js +++ b/__tests__/MenuContext-test.js @@ -1,5 +1,5 @@ import React from 'react'; -import { View } from 'react-native'; +import { View, Text } from 'react-native'; import { render } from './helpers'; import { MenuOptions, MenuTrigger } from '../src/index'; import MenuOutside from '../src/renderers/MenuOutside'; @@ -95,8 +95,8 @@ describe('MenuContext', () => { expect(backdrop).toBeFalsy(); expect(options).toBeFalsy(); expect(components.props.children).toEqual([ - , - Some text + , // eslint-disable-line react/jsx-key + Some text // eslint-disable-line react/jsx-key ]); }); @@ -106,19 +106,20 @@ describe('MenuContext', () => { ); const { menuRegistry, menuActions } = instance.getChildContext(); menuRegistry.subscribe(menu1); - menuActions.openMenu('menu1'); - expect(menuActions.isMenuOpen()).toEqual(true); - expect(menu1._getOpened()).toEqual(true); - initOutput.props.onLayout(defaultLayout); - // next render will start rendering open menu - const output = renderer.getRenderOutput(); - expect(output.props.children.length).toEqual(3); - const [ components, backdrop, options ] = output.props.children; - expect(components.type).toEqual(View); - expect(backdrop.type).toEqual(Backdrop); - expect(options.type).toEqual(MenuOutside); - // on open was called only once - expect(menu1.props.onOpen.calls.count()).toEqual(1); + menuActions.openMenu('menu1').then(() => { + expect(menuActions.isMenuOpen()).toEqual(true); + expect(menu1._getOpened()).toEqual(true); + initOutput.props.onLayout(defaultLayout); + // next render will start rendering open menu + const output = renderer.getRenderOutput(); + expect(output.props.children.length).toEqual(3); + const [ components, backdrop, options ] = output.props.children; + expect(components.type).toEqual(View); + expect(backdrop.type).toEqual(Backdrop); + expect(options.type).toEqual(MenuOutside); + // on open was called only once + expect(menu1.props.onOpen.calls.count()).toEqual(1); + }); }); it('should close menu', () => { @@ -166,13 +167,14 @@ describe('MenuContext', () => { const { menuRegistry, menuActions } = instance.getChildContext(); initOutput.props.onLayout(defaultLayout); menuRegistry.subscribe(menu1); - menuActions.openMenu('menu_not_existing'); - expect(menuActions.isMenuOpen()).toEqual(false); - const output = renderer.getRenderOutput(); - const [ components, backdrop, options ] = output.props.children; - expect(components.type).toEqual(View); - expect(backdrop).toBeFalsy(); - expect(options).toBeFalsy(); + return menuActions.openMenu('menu_not_existing').then(() => { + expect(menuActions.isMenuOpen()).toEqual(false); + const output = renderer.getRenderOutput(); + const [ components, backdrop, options ] = output.props.children; + expect(components.type).toEqual(View); + expect(backdrop).toBeFalsy(); + expect(options).toBeFalsy(); + }); }); it('should not open menu if not initialized', () => { @@ -181,13 +183,14 @@ describe('MenuContext', () => { ); const { menuRegistry, menuActions } = instance.getChildContext(); menuRegistry.subscribe(menu1); - menuActions.openMenu('menu1'); - expect(menuActions.isMenuOpen()).toEqual(true); - const [ components, backdrop, options ] = output.props.children; - // on layout has not been not called - expect(components.type).toEqual(View); - expect(backdrop).toBeFalsy(); - expect(options).toBeFalsy(); + menuActions.openMenu('menu1').then(() => { + expect(menuActions.isMenuOpen()).toEqual(true); + const [ components, backdrop, options ] = output.props.children; + // on layout has not been not called + expect(components.type).toEqual(View); + expect(backdrop).toBeFalsy(); + expect(options).toBeFalsy(); + }); }); it('should update options layout', () => { @@ -197,26 +200,27 @@ describe('MenuContext', () => { const { menuRegistry, menuActions } = instance.getChildContext(); initOutput.props.onLayout(defaultLayout); menuRegistry.subscribe(menu1); - menuActions.openMenu('menu1'); - const output = renderer.getRenderOutput(); - expect(output.props.children.length).toEqual(3); - const options = output.props.children[2]; - expect(typeof options.props.onLayout).toEqual('function'); - options.props.onLayout({ - nativeEvent: { - layout: { + menuActions.openMenu('menu1').then(() => { + const output = renderer.getRenderOutput(); + expect(output.props.children.length).toEqual(3); + const options = output.props.children[2]; + expect(typeof options.props.onLayout).toEqual('function'); + options.props.onLayout({ + nativeEvent: { + layout: { + width: 22, + height: 33 + } + } + }); + expect(menuRegistry.getMenu('menu1')).toEqual(objectContaining({ + optionsLayout: { width: 22, + isOutside: true, height: 33 } - } + })); }); - expect(menuRegistry.getMenu('menu1')).toEqual(objectContaining({ - optionsLayout: { - width: 22, - isOutside: true, - height: 33 - } - })); }); it('should render backdrop that will trigger onBackdropPress', () => { @@ -226,13 +230,14 @@ describe('MenuContext', () => { const { menuRegistry, menuActions } = instance.getChildContext(); initOutput.props.onLayout(defaultLayout); menuRegistry.subscribe(menu1); - menuActions.openMenu('menu1'); - const output = renderer.getRenderOutput(); - expect(output.props.children.length).toEqual(3); - const backdrop = output.props.children[1]; - expect(backdrop.type).toEqual(Backdrop); - backdrop.props.onPress(); - expect(menu1.props.onBackdropPress).toHaveBeenCalled(); + menuActions.openMenu('menu1').then(() => { + const output = renderer.getRenderOutput(); + expect(output.props.children.length).toEqual(3); + const backdrop = output.props.children[1]; + expect(backdrop.type).toEqual(Backdrop); + backdrop.props.onPress(); + expect(menu1.props.onBackdropPress).toHaveBeenCalled(); + }); }); }); diff --git a/package.json b/package.json index dc5545f..a9bf17b 100644 --- a/package.json +++ b/package.json @@ -21,10 +21,10 @@ }, "homepage": "https://github.com/instea/react-native-popup-menu", "jest": { - "scriptPreprocessor": "/node_modules/babel-jest", - "testFileExtensions": [ - "js" - ], + "transform": { + ".*": "/node_modules/babel-jest" + }, + "testRegex": ".*-test.js", "moduleFileExtensions": [ "js" ], @@ -41,18 +41,20 @@ }, "devDependencies": { "babel-core": "^6.8.0", - "babel-eslint": "^6.0.4", - "babel-jest": "^12.0.2", + "babel-eslint": "^7.2.3", + "babel-jest": "^20.0.1", + "babel-polyfill": "^6.23.0", "babel-preset-react": "^6.5.0", - "babel-preset-react-native": "^1.7.0", + "babel-preset-react-native": "^1.9.2", "chai": "^3.5.0", - "eslint": "^2.9.0", - "eslint-plugin-react": "^5.1.1", + "eslint": "^3.19.0", + "eslint-plugin-react": "^7.0.0", "jasmine-reporters": "^2.1.1", - "jest-cli": "^12.0.2", - "mocha": "^2.4.5", - "react": "^15.0.2", - "react-addons-test-utils": "^15.0.2", - "sinon": "^1.17.4" + "jest-cli": "^20.0.1", + "mocha": "^3.3.0", + "react": "^15.5.4", + "react-addons-test-utils": "^15.5.1", + "react-dom": "^15.5.4", + "sinon": "^2.2.0" } } diff --git a/src/MenuContext.js b/src/MenuContext.js index 39370da..daa0c58 100644 --- a/src/MenuContext.js +++ b/src/MenuContext.js @@ -40,12 +40,12 @@ export default class MenuContext extends Component { openMenu(name) { const menu = this._menuRegistry.getMenu(name); if (!menu) { - return console.warn(`menu with name ${name} does not exist`); + console.warn(`menu with name ${name} does not exist`); + return Promise.resolve(); } debug('open menu', name); menu.instance._setOpened(true); - this._notify(); - return Promise.resolve(); + return this._notify(); } closeMenu() { // has no effect on controlled menus @@ -53,7 +53,7 @@ export default class MenuContext extends Component { this._menuRegistry.getAll() .filter(menu => menu.instance._getOpened()) .forEach(menu => menu.instance._setOpened(false)); - this._notify(); + return this._notify(); } _invalidateTriggerLayouts() { @@ -79,7 +79,8 @@ export default class MenuContext extends Component { toggleMenu(name) { const menu = this._menuRegistry.getMenu(name); if (!menu) { - return console.warn(`menu with name ${name} does not exist`); + console.warn(`menu with name ${name} does not exist`); + return Promise.resolve(); } debug('toggle menu', name); if (menu.instance._getOpened()) { @@ -96,11 +97,11 @@ export default class MenuContext extends Component { // set newly opened menu before any callbacks are called this.openedMenu = next === NULL ? undefined : next; if (!forceUpdate && !this._isRenderNeeded(prev, next)) { - return; + return Promise.resolve(); } debug('notify: next menu:', next.name, ' prev menu:', prev.name); let afterSetState = undefined; - let beforeSetState = Promise.resolve; + let beforeSetState = () => Promise.resolve(); if (prev.name !== next.name) { if (prev !== NULL && !prev.instance._isOpen()) { beforeSetState = () => this._beforeClose(prev) @@ -111,7 +112,7 @@ export default class MenuContext extends Component { afterSetState = () => this._initOpen(next); } } - beforeSetState().then(() => { + return beforeSetState().then(() => { this.setState({ openedMenu: this.openedMenu }, afterSetState); debug('notify ended'); }); From 00e599e1e140d0ffe8b93f99c2ba9dbc51a27c01 Mon Sep 17 00:00:00 2001 From: Martin Bielik Date: Fri, 12 May 2017 14:53:55 +0200 Subject: [PATCH 3/3] eslint issues after lib upgrade --- .eslintrc | 1 + __tests__/.eslintrc | 3 +++ __tests__/Menu-test.js | 2 +- __tests__/MenuContext-test.js | 4 ++-- __tests__/renderers/ContextMenu-test.js | 2 +- __tests__/renderers/MenuOutside-test.js | 2 +- .../renderers/NotAnimatedContextMenu-test.js | 2 +- __tests__/renderers/SlideInMenu-test.js | 2 +- examples/MenuMethodsExample.js | 10 +++++--- examples/OriginalExample.js | 2 +- src/MenuContext.js | 24 ++++++++++++++----- 11 files changed, 37 insertions(+), 17 deletions(-) diff --git a/.eslintrc b/.eslintrc index 1dd467e..0184251 100644 --- a/.eslintrc +++ b/.eslintrc @@ -25,5 +25,6 @@ "comma-dangle": 0, "react/prop-types": 0, "react/no-did-mount-set-state": 0, + "react/no-deprecated": 0 } } diff --git a/__tests__/.eslintrc b/__tests__/.eslintrc index a2dc967..fa3163c 100644 --- a/__tests__/.eslintrc +++ b/__tests__/.eslintrc @@ -10,5 +10,8 @@ "jest": true, "expect": true, "jasmine": true + }, + "rules": { + "react/jsx-key": 0, } } diff --git a/__tests__/Menu-test.js b/__tests__/Menu-test.js index 1bc7385..ce089f5 100644 --- a/__tests__/Menu-test.js +++ b/__tests__/Menu-test.js @@ -1,5 +1,5 @@ import React from 'react'; -import { View } from 'react-native'; +import { View, Text } from 'react-native'; import { render } from './helpers'; import { MenuTrigger, MenuOptions } from '../src/index'; diff --git a/__tests__/MenuContext-test.js b/__tests__/MenuContext-test.js index bd0f4c9..76c1d24 100644 --- a/__tests__/MenuContext-test.js +++ b/__tests__/MenuContext-test.js @@ -95,8 +95,8 @@ describe('MenuContext', () => { expect(backdrop).toBeFalsy(); expect(options).toBeFalsy(); expect(components.props.children).toEqual([ - , // eslint-disable-line react/jsx-key - Some text // eslint-disable-line react/jsx-key + , + Some text ]); }); diff --git a/__tests__/renderers/ContextMenu-test.js b/__tests__/renderers/ContextMenu-test.js index 8e97586..6b83a78 100644 --- a/__tests__/renderers/ContextMenu-test.js +++ b/__tests__/renderers/ContextMenu-test.js @@ -1,5 +1,5 @@ import React from 'react'; -import { Animated } from 'react-native'; +import { Animated, Text } from 'react-native'; import { render } from '../helpers'; jest.dontMock('../../src/renderers/ContextMenu'); diff --git a/__tests__/renderers/MenuOutside-test.js b/__tests__/renderers/MenuOutside-test.js index 959233a..15f5da7 100644 --- a/__tests__/renderers/MenuOutside-test.js +++ b/__tests__/renderers/MenuOutside-test.js @@ -1,5 +1,5 @@ import React from 'react'; -import { View } from 'react-native'; +import { View, Text } from 'react-native'; import { render } from '../helpers'; jest.dontMock('../../src/renderers/MenuOutside'); diff --git a/__tests__/renderers/NotAnimatedContextMenu-test.js b/__tests__/renderers/NotAnimatedContextMenu-test.js index fb6c772..553adc1 100644 --- a/__tests__/renderers/NotAnimatedContextMenu-test.js +++ b/__tests__/renderers/NotAnimatedContextMenu-test.js @@ -1,5 +1,5 @@ import React from 'react'; -import { View } from 'react-native'; +import { View, Text } from 'react-native'; import { render } from '../helpers'; jest.dontMock('../../src/renderers/NotAnimatedContextMenu'); diff --git a/__tests__/renderers/SlideInMenu-test.js b/__tests__/renderers/SlideInMenu-test.js index 4ff0790..b15f5f0 100644 --- a/__tests__/renderers/SlideInMenu-test.js +++ b/__tests__/renderers/SlideInMenu-test.js @@ -1,5 +1,5 @@ import React from 'react'; -import { Animated } from 'react-native'; +import { Animated, Text } from 'react-native'; import { render } from '../helpers'; jest.dontMock('../../src/renderers/SlideInMenu'); diff --git a/examples/MenuMethodsExample.js b/examples/MenuMethodsExample.js index c5e506b..a32c635 100644 --- a/examples/MenuMethodsExample.js +++ b/examples/MenuMethodsExample.js @@ -12,19 +12,23 @@ export default class ControlledExample extends Component { onOptionSelect(value) { alert(`Selected number: ${value}`); if (value === 1) { - this.refs.menu.close(); + this.menu.close(); } return false; } openMenu() { - this.refs.menu.open(); + this.menu.open(); + } + + onRef = r => { + this.menu = r; } render() { return ( - this.onOptionSelect(value)} ref='menu'> + this.onOptionSelect(value)} ref={this.onRef}> diff --git a/examples/OriginalExample.js b/examples/OriginalExample.js index fb8a20d..bde0302 100644 --- a/examples/OriginalExample.js +++ b/examples/OriginalExample.js @@ -46,7 +46,7 @@ const OriginalExample = React.createClass({ }); }, render() { - return ( + return ( // eslint-disable-next-line react/no-string-refs diff --git a/src/MenuContext.js b/src/MenuContext.js index daa0c58..0ed51b5 100644 --- a/src/MenuContext.js +++ b/src/MenuContext.js @@ -68,10 +68,10 @@ export default class MenuContext extends Component { _beforeClose(menu) { debug('before close', menu.name); - const hideMenu = (this.refs.menuOptions - && this.refs.menuOptions.close - && this.refs.menuOptions.close()) || Promise.resolve(); - const hideBackdrop = this.refs.backdrop && this.refs.backdrop.close(); + const hideMenu = (this.optionsRef + && this.optionsRef.close + && this.optionsRef.close()) || Promise.resolve(); + const hideBackdrop = this.backdropRef && this.backdropRef.close(); this._invalidateTriggerLayouts(); return Promise.all([hideMenu, hideBackdrop]); } @@ -147,7 +147,11 @@ export default class MenuContext extends Component { { this.props.children } {shouldRenderMenu && - this._onBackdropPress()} style={customStyles.backdrop} ref='backdrop' /> + this._onBackdropPress()} + style={customStyles.backdrop} + ref={this.onBackdropRef} + /> } {shouldRenderMenu && this._makeOptions(this.state.openedMenu) @@ -156,6 +160,14 @@ export default class MenuContext extends Component { ); } + onBackdropRef = r => { + this.backdropRef = r; + } + + onOptionsRef = r => { + this.optionsRef = r; + } + _onBackdropPress() { debug('on backdrop press'); this.state.openedMenu.instance.props.onBackdropPress(); @@ -197,7 +209,7 @@ export default class MenuContext extends Component { const props = { style, onLayout, layouts }; const optionsType = isOutside ? MenuOutside : renderer; if (!isFunctional(optionsType)) { - props.ref = 'menuOptions'; + props.ref = this.onOptionsRef; } return React.createElement(optionsType, props, optionsRenderer(options)); }