Merge pull request #517 from primer/details-event-fix

Only close Details on click outside

Author: Emily

src/Details.js

@@ -1,4 +1,4 @@
-import React, {useState, useEffect} from 'react'
+import React, {useState, useEffect, useRef} from 'react'
 import PropTypes from 'prop-types'
 import styled from 'styled-components'
 import {COMMON} from './constants'
@@ -18,29 +18,31 @@ function getRenderer(children) {
 
 function DetailsBase({children, overlay, render = getRenderer(children), defaultOpen = false, ...rest}) {
   const [open, setOpen] = useState(defaultOpen)
+  const ref = useRef(null)
 
-  useEffect(
-    () => {
-      if (overlay && open) {
-        document.addEventListener('click', closeMenu)
-        return () => {
-          document.removeEventListener('click', closeMenu)
-        }
+  useEffect(() => {
+    if (overlay && open) {
+      document.addEventListener('click', closeMenu)
+      return () => {
+        document.removeEventListener('click', closeMenu)
       }
-    },
-    [open, overlay]
-  )
+    }
+  }, [open, overlay])
 
   function toggle(event) {
     setOpen(event.target.open)
   }
 
   function closeMenu(event) {
-    setOpen(false)
+    // only close the menu if we're clicking outside
+    if (event && event.target.closest('details') !== ref.current) {
+      setOpen(false)
+      document.removeEventListener('click', closeMenu)
+    }
   }
 
   return (
-    <DetailsReset {...rest} open={open} onToggle={toggle} overlay={overlay}>
+    <DetailsReset {...rest} ref={ref} open={open} onToggle={toggle} overlay={overlay}>
       {render({open})}
     </DetailsReset>
   )

src/__tests__/Details.js

@@ -40,6 +40,47 @@ describe('Details', () => {
     expect(dom.hasAttribute('open')).toEqual(false)
     expect(summary.text()).toEqual('open')
 
+    summary.simulate('click')
+  })
+
+  it('Toggles when you click outside', () => {
+    const wrapper = mount(<Details>{({open}) => <summary>{open ? 'close' : 'open'}</summary>}</Details>)
+
+    document.body.click()
+
+    const dom = wrapper.getDOMNode()
+    const summary = wrapper.find('summary')
+
+    expect(dom.hasAttribute('open')).toEqual(false)
+    expect(summary.text()).toEqual('open')
+
+    wrapper.unmount()
+  })
+
+  xit('Does not toggle or prevent click events when you click inside', () => {
+    const wrapper = mount(
+      <Details open>
+        {({open}) => (
+          <>
+            <summary>{open ? 'close' : 'open'}</summary>
+            <div>content</div>
+          </>
+        )}
+      </Details>
+    )
+
+    document.body.click()
+
+    const dom = wrapper.getDOMNode()
+    const content = dom.querySelector('div')
+
+    let clickEvent
+    content.addEventListener('click', event => (clickEvent = event))
+    content.click()
+
+    expect(dom.hasAttribute('open')).toEqual(true)
+    expect(clickEvent.defaultPrevented).toBe(false)
+
     wrapper.unmount()
   })
 })

src/__tests__/Dropdown.js

@@ -38,18 +38,18 @@ describe('Dropdown.Menu', () => {
     expect(
       render(
         <Dropdown.Menu>
-          <li>1</li>
-          <li>2</li>
-          <li>3</li>
+          <li key="a">1</li>
+          <li key="b">2</li>
+          <li key="c">3</li>
         </Dropdown.Menu>
       )
     ).toMatchSnapshot()
     expect(
       render(
         <Dropdown.Menu>
-          <li>1</li>
-          <li>2</li>
-          <li>3</li>
+          <li key={1}>1</li>
+          <li key={2}>2</li>
+          <li key={3}>3</li>
         </Dropdown.Menu>
       )
     ).toMatchSnapshot()