fix(Popup): call onOpenChange on all user actions

CHANGELOG.md

@@ -41,6 +41,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Fix multiple React's warnings about keys in docs @layershifter ([#602](https://github.com/stardust-ui/react/pull/602))
 - Fix incorrect handling of `isFromKeyboard` in `Menu` @layershifter ([#596](https://github.com/stardust-ui/react/pull/596))
 - Fix property names used in shorthand factories @kuzhelov ([#591](https://github.com/stardust-ui/react/pull/591))
+- Call `Popup` `onOpenChange` on all user initiated events @levithomason ([#619](https://github.com/stardust-ui/react/pull/619))
 
 ### Features
 - `Ref` components uses `forwardRef` API by default @layershifter ([#491](https://github.com/stardust-ui/react/pull/491))

docs/src/examples/components/Popup/Types/PopupControlledExample.shorthand.tsx

@@ -1,22 +1,17 @@
 import React from 'react'
 import { Button, Input, Popup } from '@stardust-ui/react'
 
-class PopupControlledExample extends React.Component<any, any> {
-  state = { popupOpen: false }
+class PopupControlledExample extends React.Component {
+  state = { open: false }
 
-  togglePopup() {
-    this.setState(prev => ({ popupOpen: !prev.popupOpen }))
-  }
+  handleOpenChange = (e, { open }) => this.setState({ open })
 
   render() {
     return (
       <Popup
-        open={this.state.popupOpen}
-        onOpenChange={(e, newProps) => {
-          alert(`Popup is requested to change its open state to "${newProps.open}".`)
-          this.setState({ popupOpen: newProps.open })
-        }}
-        trigger={<Button icon="expand" onClick={() => this.togglePopup()} />}
+        open={this.state.open}
+        onOpenChange={this.handleOpenChange}
+        trigger={<Button icon="expand" />}
         content={{ content: <Input icon="search" placeholder="Search..." /> }}
       />
     )

docs/src/examples/components/Popup/Types/PopupControlledExample.tsx

@@ -1,24 +1,19 @@
 import React from 'react'
 import { Button, Input, Popup } from '@stardust-ui/react'
 
-class PopupControlledExample extends React.Component<any, any> {
-  state = { popupOpen: false }
+class PopupControlledExample extends React.Component {
+  state = { open: false }
 
-  togglePopup() {
-    this.setState(prev => ({ popupOpen: !prev.popupOpen }))
-  }
+  handleOpenChange = (e, { open }) => this.setState({ open })
 
   render() {
     return (
       <Popup
-        open={this.state.popupOpen}
-        onOpenChange={(e, newProps) => {
-          alert(`Popup is requested to change its open state to "${newProps.open}".`)
-          this.setState({ popupOpen: newProps.open })
-        }}
+        open={this.state.open}
+        onOpenChange={this.handleOpenChange}
         content={{ content: <Input icon="search" placeholder="Search..." /> }}
       >
-        <Button icon="expand" onClick={() => this.togglePopup()} />
+        <Button icon="expand" />
       </Popup>
     )
   }

src/components/Popup/Popup.tsx

@@ -143,7 +143,7 @@ export default class Popup extends AutoControlledComponent<Extendable<PopupProps
 
   protected actionHandlers: AccessibilityActionHandlers = {
     toggle: e => {
-      this.trySetOpen(!this.state.open, e, true)
+      this.trySetOpen(!this.state.open, e)
     },
     closeAndFocusTrigger: e => {
       this.closeAndFocusTrigger(e)
@@ -153,7 +153,7 @@ export default class Popup extends AutoControlledComponent<Extendable<PopupProps
 
   private closeAndFocusTrigger = e => {
     if (this.state.open) {
-      this.trySetOpen(false, e, true)
+      this.trySetOpen(false, e)
       _.invoke(this.triggerDomElement, 'focus')
     }
   }
@@ -172,7 +172,7 @@ export default class Popup extends AutoControlledComponent<Extendable<PopupProps
               this.triggerDomElement && !this.triggerDomElement.contains(e.target)
 
             if (isOutsidePopupElement && isOutsideTriggerElement) {
-              this.state.open && this.trySetOpen(false, e, true)
+              this.state.open && this.trySetOpen(false, e)
             }
           },
           {
@@ -326,10 +326,9 @@ export default class Popup extends AutoControlledComponent<Extendable<PopupProps
     )
   }
 
-  private trySetOpen(newValue: boolean, eventArgs: any, forceChangeEvent: boolean = false) {
-    if (this.trySetState({ open: newValue }) || forceChangeEvent) {
-      _.invoke(this.props, 'onOpenChange', eventArgs, { ...this.props, ...{ open: newValue } })
-    }
+  private trySetOpen(newValue: boolean, eventArgs: any) {
+    this.trySetState({ open: newValue })
+    _.invoke(this.props, 'onOpenChange', eventArgs, { ...this.props, ...{ open: newValue } })
   }
 
   private applyRtlToOffset(offset: string, rtl: boolean, position: Position): string {

src/lib/AutoControlledComponent.tsx

@@ -188,7 +188,7 @@ export default class AutoControlledComponent<P = {}, S = {}> extends UIComponent
    * @param {object} maybeState State that corresponds to controlled props.
    * @param {object} [state] Actual state, useful when you also need to setState.
    */
-  trySetState = (maybeState, state?): boolean => {
+  trySetState = (maybeState, state?) => {
     const { autoControlledProps } = this.constructor as any
     if (process.env.NODE_ENV !== 'production') {
       const { name } = this.constructor
@@ -218,11 +218,6 @@ export default class AutoControlledComponent<P = {}, S = {}> extends UIComponent
 
     if (state) newState = { ...newState, ...state }
 
-    if (Object.keys(newState).length > 0) {
-      this.setState(newState)
-      return true
-    }
-
-    return false
+    if (Object.keys(newState).length > 0) this.setState(newState)
   }
 }

test/specs/components/Popup/Popup-test.tsx

@@ -1,6 +1,10 @@
-import computePopupPlacement, { Position, Alignment } from 'src/components/Popup/positioningHelper'
+import * as React from 'react'
 import { Placement } from 'popper.js'
 
+import Popup from 'src/components/Popup/Popup'
+import computePopupPlacement, { Position, Alignment } from 'src/components/Popup/positioningHelper'
+import { mountWithProvider } from '../../../utils'
+
 type PositionTestInput = {
   align: Alignment
   position: Position
@@ -56,4 +60,29 @@ describe('Popup', () => {
     testPopupPositionInRtl({ position: 'after', align: 'center', expectedPlacement: 'left' })
     testPopupPositionInRtl({ position: 'after', align: 'bottom', expectedPlacement: 'left-end' })
   })
+
+  describe('onOpenChange', () => {
+    test('is called on click', () => {
+      const spy = jest.fn()
+
+      mountWithProvider(<Popup trigger={<button />} content="Hi" onOpenChange={spy} />)
+        .find('button')
+        .simulate('click')
+
+      expect(spy).toHaveBeenCalledTimes(1)
+      expect(spy.mock.calls[0][1]).toMatchObject({ open: true })
+    })
+
+    // https://github.com/stardust-ui/react/pull/619
+    test('is called on click when controlled', () => {
+      const spy = jest.fn()
+
+      mountWithProvider(<Popup open={false} trigger={<button />} content="Hi" onOpenChange={spy} />)
+        .find('button')
+        .simulate('click')
+
+      expect(spy).toHaveBeenCalledTimes(1)
+      expect(spy.mock.calls[0][1]).toMatchObject({ open: true })
+    })
+  })
 })