Modal close refactor

migration-guides/v6.0.0.md

@@ -10,7 +10,8 @@ This version contains the following breaking changes:
 7. `<CloudinaryFileInput />` and `<FileInput />` components no longer accept an `onLoad` prop
 8. The `previewComponent` for a file input no longer receives a `value` prop and `file` is a file object (with the url)
 9. The tag on `<Spinner />` now uses the class `spinner` in place of an id and supports additional classes
-10. `<TabBar />` now expects both `options` and `value` as required props.
+10. `<TabBar />` now expects both `options` and `value` as required props
+11. `<Modal />` no longer accepts the `hideCloseButton` prop
 
 The required changes for each item are detailed below.
 
@@ -196,7 +197,7 @@ const PreviewComponent = ({ file }) => {
 <FileInput previewComponent={PreviewComponent} />
 ```
 
-#### 9. The tag on `<Spinner />` now uses the `spinner` class in place of an id
+## 9. The tag on `<Spinner />` now uses the `spinner` class in place of an id
 
 Replace any styling rules that target `#spinner` with `.spinner`.
 
@@ -215,7 +216,7 @@ Replace any styling rules that target `#spinner` with `.spinner`.
   }
 }
 ```
-#### 10. `<TabBar />` now expects both `options` and `value` as required props.
+## 10. `<TabBar />` now expects both `options` and `value` as required props.
 
 Make sure that any instances of `<TabBar />` in your application are already sending these two props.
 
@@ -225,3 +226,22 @@ Make sure that any instances of `<TabBar />` in your application are already sen
     value={currentTab}
   />
 ```
+## 11. `<Modal />` no longer accepts the `hideCloseButton` prop.
+Replace `hideCloseButton` with `preventClose`. If you still need the modal to close on escape and/or by clicking the overlay, you can manually set those props. By default, `shouldCloseOnEsc` and `shouldCloseOnOverlayClick` will be set to the opposite of `preventClose` (default `false`).
+
+```jsx
+// Before
+<Modal hideCloseButton>
+  Modal content
+</Modal>
+
+// After
+<Modal preventClose>
+  Modal content
+</Modal>
+
+// With prop override
+<Modal preventClose shouldCloseOnEsc={true}>
+  Modal content
+</Modal>
+```

src/modal.js

@@ -13,7 +13,7 @@ import { isServer } from './utils'
  * @name Modal
  * @type Function
  * @param {Function} onClose - A handler for closing the modal. May be triggered via the close button, and outside click, or a key press.
- * @param {Boolean} [hideCloseButton] - A flag for hiding the default close button.
+ * @param {Boolean} [preventClose=false] - A flag for preventing the modal from being closed (close button, escape, or overlay click).
  *
  * @example
  *
@@ -40,12 +40,12 @@ import { isServer } from './utils'
 
 const propTypes = {
   onClose: PropTypes.func.isRequired,
-  hideCloseButton: PropTypes.bool,
+  preventClose: PropTypes.bool,
   children: PropTypes.node,
 }
 
 const defaultProps = {
-  hideCloseButton: false,
+  preventClose: false,
 }
 
 function getRootElement() {
@@ -57,23 +57,27 @@ function getRootElement() {
 
 // A wrapper for react-modal that adds some styles and a close button.
 // See https://github.com/reactjs/react-modal for usage.
-function Modal({ onClose, hideCloseButton, children, ...rest }) {
+function Modal({ onClose, preventClose, children, ...rest }) {
+  const canClose = !preventClose
   return (
     <ReactModal
-      isOpen
+      isOpen={true}
       onRequestClose={onClose}
       portalClassName="modal"
       className="modal-inner"
       overlayClassName="modal-fade-screen"
       bodyOpenClassName="modal-open"
       appElement={getRootElement()}
       ariaHideApp={isServer()} // Opt out of setting appElement on the server.
+      shouldCloseOnEsc={canClose}
+      shouldCloseOnOverlayClick={canClose}
       {...rest}
     >
       <div className="modal-content">{children}</div>
-      {!!onClose && !hideCloseButton && (
+      {canClose && (
         <>
           <button
+            type="button"
             onClick={onClose}
             className="modal-close"
             aria-label="Close Modal"

stories/modal.story.js

@@ -16,12 +16,12 @@ storiesOf('Modal', module)
       </div>
     )
   })
-  .add('without close button', () => {
+  .add('with close prevented', () => {
     const [modalShown, setModalShown] = useState(true)
     return (
       <div>
         {modalShown && (
-          <Modal onClose={() => setModalShown(false)} hideCloseButton>
+          <Modal onClose={() => setModalShown(false)} preventClose={true}>
             This is the modal content!
           </Modal>
         )}

test/modal.test.js

@@ -3,24 +3,51 @@ import { mount } from 'enzyme'
 import { Modal } from '../src/'
 import { noop } from 'lodash'
 
-test('Modal is shown by default', () => {
-  const wrapper = mount(<Modal onClose={noop} />)
-  expect(wrapper.find('.modal-content').exists()).toEqual(true)
-})
+describe('Modal', () => {
+  test('is shown by default', () => {
+    const wrapper = mount(<Modal onClose={noop} />)
+    expect(wrapper.find('.modal-content').exists()).toEqual(true)
+  })
+
+  test('can be hidden/animated by manually passing isOpen', () => {
+    const wrapper = mount(<Modal isOpen={false} onClose={noop} />)
+    expect(wrapper.find('.modal-content').exists()).toEqual(false)
+  })
+
+  test('calls close handler when close button is clicked', () => {
+    const onClose = jest.fn()
+    const wrapper = mount(<Modal onClose={onClose} />)
+    wrapper.find('.modal-close').simulate('click')
+    expect(onClose).toHaveBeenCalled()
+  })
 
-test('Modal can be hidden/animated by manually passing isOpen', () => {
-  const wrapper = mount(<Modal isOpen={false} onClose={noop} />)
-  expect(wrapper.find('.modal-content').exists()).toEqual(false)
-})
+  describe('when preventClose=true', () => {
+    test('hides close button', () => {
+      const wrapper = mount(<Modal preventClose={true} onClose={noop} />)
+      expect(wrapper.find('.modal-close').exists()).toEqual(false)
+    })
 
-test('Modal calls close handler when close button is clicked', () => {
-  const onClose = jest.fn()
-  const wrapper = mount(<Modal onClose={onClose} />)
-  wrapper.find('.modal-close').simulate('click')
-  expect(onClose).toHaveBeenCalled()
-})
+    test('does not close by escape key', () => {
+      const onClose = jest.fn()
+      const wrapper = mount(<Modal preventClose={true} onClose={onClose} />)
+      wrapper.find('.modal-content').simulate('keydown', { keyCode: 27 })
+      expect(onClose).not.toHaveBeenCalled()
+    })
+
+    test('does not close by overlay click', () => {
+      const onClose = jest.fn()
+      const overlayClass = 'test-overlay'
+      const wrapper = mount(<Modal preventClose={true} onClose={onClose} overlayClassName={overlayClass} />)
+      wrapper.find('.' + overlayClass).simulate('click')
+      expect(onClose).not.toHaveBeenCalled()
+      expect(wrapper.find('.modal-content').exists()).toEqual(true)
+    })
 
-test('Modal hides close button when hideCloseButton=true', () => {
-  const wrapper = mount(<Modal hideCloseButton onClose={noop} />)
-  expect(wrapper.find('.modal-close').exists()).toEqual(false)
+    test('allows individual prop overrides', () => {
+      const onClose = jest.fn()
+      const wrapper = mount(<Modal preventClose={true} onClose={onClose} shouldCloseOnEsc={true} />)
+      wrapper.find('.modal-content').simulate('keydown', { keyCode: 27 })
+      expect(onClose).toHaveBeenCalled()
+    })
+  })
 })