fix: popover with openOnClick

When openOnClick is used, the popover should close when clicked outside the popover. Changed Popover to use Tippy trigger property instead of a custom state with triggers to support click behaviour.
42BV · Dec 12, 2024 · 94e7f1f · 94e7f1f
1 parent 98c8cd3
commit 94e7f1f
Show file tree

Hide file tree

Showing 4 changed files with 132 additions and 63 deletions.
diff --git a/src/core/Popover/Popover.stories.tsx b/src/core/Popover/Popover.stories.tsx
@@ -17,9 +17,14 @@ storiesOf('core/Popover', module)
   .addDecorator((Story) => (
     <>
       <Alert color="warning" className="mb-4">
-        <p className="mb-0">To be able to use Popover, you have to add @tippyjs/react to your dependencies:</p>
+        <p className="mb-0">
+          To be able to use Popover, you have to add @tippyjs/react to your
+          dependencies:
+        </p>
         <code>npm install --save @tippyjs/react</code>
-        <p className="mb-0 mt-2">You also have to add the stylesheet to your project</p>
+        <p className="mb-0 mt-2">
+          You also have to add the stylesheet to your project
+        </p>
         <code>@import &apos;tippy.js/dist/tippy.css&apos;;</code>
       </Alert>
       <Story />
@@ -41,34 +46,61 @@ storiesOf('core/Popover', module)
             <NiceCard />
           </Popover>
 
-          <Popover target={<><Icon icon="map" /> Hover this icon!</>}>
+          <Popover
+            target={
+              <>
+                <Icon icon="map" /> Hover this icon!
+              </>
+            }
+          >
             <TinyCrud />
           </Popover>
         </Col>
       </Row>
     </div>
   ))
 
+  .add('open on click', () => {
+    return (
+      <Card>
+        <Popover
+          openOnClick={true}
+          target="Open"
+          tag="div"
+          className="text-center"
+        >
+          <TinyCrud />
+        </Popover>
+      </Card>
+    );
+  })
+
   .add('taking control', () => {
-    const [ isOpen, setIsOpen ] = useState(false);
+    const [isOpen, setIsOpen] = useState(false);
 
     return (
       <Card>
         Status: {isOpen ? 'opened' : 'closed'}
-        <Popover isOpen={isOpen} target="Open" tag="div" className="text-center">
+        <Popover
+          isOpen={isOpen}
+          target="Open"
+          tag="div"
+          className="text-center"
+        >
           <TinyCrud />
         </Popover>
         <Button onClick={() => setIsOpen(!isOpen)}>Show / hide</Button>
         <p className="mt-4 mb-0">
-          Note: you can take complete control over the Popover by using the <code>isOpen</code> prop.
-          Once you make it <code>true</code> or <code>false</code> the hover behavior will be disabled.
+          Note: you can take complete control over the Popover by using the{' '}
+          <code>isOpen</code> prop. Once you make it <code>true</code> or{' '}
+          <code>false</code> the hover behavior will be disabled.
         </p>
       </Card>
     );
   })
 
   .add('on click outside', () => {
-    const [ isOpen, setIsOpen ] = useState(false);
+    const [isOpen, setIsOpen] = useState(false);
 
     return (
       <Card>
@@ -84,12 +116,13 @@ storiesOf('core/Popover', module)
         </Popover>
         <Button onClick={() => setIsOpen(true)}>Show</Button>
         <p className="mt-4">
-          Note: you can take complete control over the Popover by using the <code>isOpen</code> prop.
-          Once you make it <code>true</code> or <code>false</code> the hover behavior will be disabled.
+          Note: you can take complete control over the Popover by using the{' '}
+          <code>isOpen</code> prop. Once you make it <code>true</code> or{' '}
+          <code>false</code> the hover behavior will be disabled.
         </p>
         <p>
-          In combination with <code>onClickOutside</code> you can
-          close the popover when clicked anywhere outside the popover.
+          In combination with <code>onClickOutside</code> you can close the
+          popover when clicked anywhere outside the popover.
         </p>
       </Card>
     );
@@ -133,15 +166,24 @@ storiesOf('core/Popover', module)
       <span className="d-block fs-5">Alignment-modifier</span>
       <Row className="mt-3">
         <Col className="d-flex justify-content-around">
-          <Popover target={<div className="py-5 px-3 border"> Hover me! </div>} placement="right-start">
+          <Popover
+            target={<div className="py-5 px-3 border"> Hover me! </div>}
+            placement="right-start"
+          >
             right-start
           </Popover>
 
-          <Popover target={<div className="py-5 px-3 border"> Hover me! </div>} placement="right">
+          <Popover
+            target={<div className="py-5 px-3 border"> Hover me! </div>}
+            placement="right"
+          >
             right
           </Popover>
 
-          <Popover target={<div className="py-5 px-3 border"> Hover me! </div>} placement="right-end">
+          <Popover
+            target={<div className="py-5 px-3 border"> Hover me! </div>}
+            placement="right-end"
+          >
             right-end
           </Popover>
         </Col>
@@ -212,7 +254,11 @@ storiesOf('core/Popover', module)
             You can change that with the tag property
           </Popover>
 
-          <Popover target="My target is in a <button>" tag="button" className="btn btn-primary">
+          <Popover
+            target="My target is in a <button>"
+            tag="button"
+            className="btn btn-primary"
+          >
             Buttons are allowed too!
           </Popover>
         </Col>
@@ -221,9 +267,9 @@ storiesOf('core/Popover', module)
   ));
 
 function TinyCrud() {
-  const [ open, setOpen ] = useState(false);
+  const [open, setOpen] = useState(false);
 
-  const persons = [ 'aap', 'noot', 'mies' ];
+  const persons = ['aap', 'noot', 'mies'];
 
   return (
     <ListGroup style={{ width: 300 }}>

diff --git a/src/core/Popover/Popover.test.tsx b/src/core/Popover/Popover.test.tsx
@@ -72,17 +72,6 @@ describe('Component: Popover', () => {
   });
 
   describe('events', () => {
-    it('should call onClick when tag is clicked', () => {
-      const onClickSpy = jest.fn();
-      render(
-        <Popover onClick={onClickSpy} target={<>Click this</>} tag="button">
-          Popover content
-        </Popover>
-      );
-      fireEvent.click(screen.getByText('Click this'));
-      expect(onClickSpy).toBeCalledTimes(1);
-    });
-
     it('should open on click when openOnClick is true', () => {
       render(
         <Popover openOnClick={true} target={<>Click this</>} tag="button">

diff --git a/src/core/Popover/Popover.tsx b/src/core/Popover/Popover.tsx
@@ -1,4 +1,4 @@
-import React, { CSSProperties, useEffect, useState } from 'react';
+import React, { CSSProperties } from 'react';
 import Tippy from '@tippyjs/react';
 import { TippyPlacement } from '../types';
 
@@ -17,14 +17,6 @@ type Props = {
    */
   openOnClick?: boolean;
 
-  /**
-   * Optional callback that gets triggered when the tag is clicked.
-   * When openOnClick is true, this component will take care of its visibility
-   * state, unless onClick is defined.
-   * This should be used when wanting to take complete control over the popover.
-   */
-  onClick?: () => void;
-
   /**
    * Optionally callback that gets triggered when clicked outside the popover.
    * Is useful for when wanting to take complete control over the popover.
@@ -97,30 +89,15 @@ export function Popover({
   className,
   isOpen,
   openOnClick,
-  onClick,
   onClickOutside,
   style,
   maxWidth
 }: Props) {
   const Tag = tag;
 
-  const [visible, setVisible] = useState(openOnClick ? !!isOpen : undefined);
-
-  useEffect(() => {
-    setVisible(openOnClick ? !!isOpen : undefined);
-  }, [openOnClick, isOpen]);
-
-  function tagClicked() {
-    if (onClick) {
-      onClick();
-    } else if (openOnClick) {
-      setVisible(!visible);
-    }
-  }
-
   return (
     <Tippy
-      visible={visible}
+      visible={isOpen}
       onClickOutside={onClickOutside}
       className="border-0 tippy-popover"
       content={children}
@@ -129,14 +106,9 @@ export function Popover({
       interactive={true}
       zIndex={1049} // One level below bootstrap's modal
       maxWidth={maxWidth}
+      trigger={openOnClick ? 'click' : 'mouseenter focus'}
     >
-      <Tag
-        className={className}
-        style={style}
-        tabIndex={0}
-        role="button"
-        onClick={tagClicked}
-      >
+      <Tag className={className} style={style} tabIndex={0} role="button">
         {target}
       </Tag>
     </Tippy>

diff --git a/src/core/Popover/__snapshots__/Popover.test.tsx.snap b/src/core/Popover/__snapshots__/Popover.test.tsx.snap
@@ -17,21 +17,52 @@ exports[`Component: Popover ui default 1`] = `
 exports[`Component: Popover ui open 1`] = `
 <div>
   <span
-    aria-expanded="false"
+    aria-expanded="true"
     role="button"
     tabindex="0"
   >
     <div>
       The popover should be wrapped around this div, in a span
     </div>
   </span>
+  <div
+    data-tippy-root=""
+    id="tippy-2"
+    style="z-index: 1049; visibility: visible; transition: none; position: absolute; left: 0px; top: 0px; margin: 0px; bottom: 0px; transform: translate(0px, -7px);"
+  >
+    <div
+      class="tippy-box border-0 tippy-popover"
+      data-animation="fade"
+      data-escaped=""
+      data-placement="top"
+      data-reference-hidden=""
+      data-state="hidden"
+      role="tooltip"
+      style="max-width: 350px; transition-duration: 0ms;"
+      tabindex="-1"
+    >
+      <div
+        class="tippy-content"
+        data-state="hidden"
+        style="transition-duration: 0ms;"
+      >
+        <div>
+          Popover content
+        </div>
+      </div>
+      <div
+        class="tippy-arrow"
+        style="position: absolute; left: 0px; transform: translate(3px, 0px);"
+      />
+    </div>
+  </div>
 </div>
 `;
 
 exports[`Component: Popover ui with optional properties 1`] = `
 <div>
   <span
-    aria-expanded="false"
+    aria-expanded="true"
     role="button"
     style="margin-top: 5px; padding: 10px;"
     tabindex="0"
@@ -40,5 +71,36 @@ exports[`Component: Popover ui with optional properties 1`] = `
       The popover should be wrapped around this div, in a div
     </div>
   </span>
+  <div
+    data-tippy-root=""
+    id="tippy-5"
+    style="z-index: 1049; visibility: visible; transition: none; position: absolute; left: 0px; top: 0px; margin: 0px; transform: translate(0px, 10px);"
+  >
+    <div
+      class="tippy-box border-0 tippy-popover"
+      data-animation="fade"
+      data-escaped=""
+      data-placement="bottom"
+      data-reference-hidden=""
+      data-state="hidden"
+      role="tooltip"
+      style="max-width: 350px; transition-duration: 0ms;"
+      tabindex="-1"
+    >
+      <div
+        class="tippy-content"
+        data-state="hidden"
+        style="transition-duration: 0ms;"
+      >
+        <div>
+          Popover content
+        </div>
+      </div>
+      <div
+        class="tippy-arrow"
+        style="position: absolute; left: 0px; transform: translate(3px, 0px);"
+      />
+    </div>
+  </div>
 </div>
 `;