refactor: code review feedbacks

AxaFrance · Sep 1, 2020 · fee2f9a · fee2f9a
1 parent f486aa5
commit fee2f9a
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 43 deletions.
diff --git a/packages/help/src/__snapshots__/Help.spec.js.snap b/packages/help/src/__snapshots__/Help.spec.js.snap
@@ -1,12 +1,14 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
 exports[`<Help> Render Should render Help component 1`] = `
-<button
+<div
   className="af-popover__wrapper"
   onClick={[Function]}
+  onKeyDown={[Function]}
   onMouseEnter={[Function]}
   onMouseLeave={[Function]}
-  type="button"
+  role="button"
+  tabIndex="0"
 >
   <div
     className="af-popover__container"
@@ -27,5 +29,5 @@ exports[`<Help> Render Should render Help component 1`] = `
       </button>
     </div>
   </div>
-</button>
+</div>
 `;
diff --git a/packages/popover/src/Popover.js b/packages/popover/src/Popover.js
@@ -1,4 +1,4 @@
-import React, { useEffect, useRef, useState } from 'react';
+import React, { useRef, useState } from 'react';
 import PropTypes from 'prop-types';
 import { Constants } from '@axa-fr/react-toolkit-core';
 import PopoverBase from './PopoverBase';
@@ -30,73 +30,93 @@ export const PopoverClick = props => {
   const wrapperRef = useRef(null);
   const [isOpen, setOpen] = useState(false);
   const [isHover, setHover] = useState(false);
+  const [isPopoverHover, setPopoverHover] = useState(false);
 
-  useEffect(() => {
-    const handleClickOutside = event => {
-      if (wrapperRef.current && !wrapperRef.current.contains(event.target)) {
-        setOpen(false);
-      }
-    };
-
-    document.addEventListener('click', handleClickOutside);
-    return () => {
+  const handleClickOutside = event => {
+    if (wrapperRef.current && !wrapperRef.current.contains(event.target)) {
+      setOpen(false);
       document.removeEventListener('click', handleClickOutside);
-    };
-  }, [wrapperRef]);
+    }
+  };
+
+  const handleClick = event => {
+    if (isPopoverHover) {
+      event.stopPropagation();
+      return;
+    }
 
-  const click = () => {
-    setOpen(!isOpen && isHover);
+    const shouldOpen = !isOpen && isHover;
+    setOpen(shouldOpen);
+
+    if (shouldOpen) {
+      document.addEventListener('click', handleClickOutside);
+    }
   };
 
-  const enter = () => {
+  const handleMouseEnter = () => {
     setHover(true);
   };
 
-  const leave = () => {
+  const handleMouseLeave = () => {
     setHover(false);
   };
 
+  const handleMouseEnterPopover = () => {
+    setPopoverHover(true);
+  };
+
+  const handleMouseLeavePopover = () => {
+    setPopoverHover(false);
+  };
+
   return (
-    <button
+    <div
+      role="button"
+      tabIndex="0"
       ref={wrapperRef}
       className="af-popover__wrapper"
-      type="button"
-      onMouseEnter={enter}
-      onMouseLeave={leave}
-      onClick={click}>
+      onMouseEnter={handleMouseEnter}
+      onMouseLeave={handleMouseLeave}
+      onKeyDown={handleClick}
+      onClick={handleClick}>
       <PopoverBase
+        onMouseEnter={handleMouseEnterPopover}
+        onMouseLeave={handleMouseLeavePopover}
         isOpen={isOpen}
         placement={placement}
         className={className}
         classModifier={classModifier}>
         {children}
       </PopoverBase>
-    </button>
+    </div>
   );
 };
 
 export const PopoverOver = props => {
   const { children, placement, className, classModifier } = props;
   const [isOpen, setOpen] = useState(false);
 
-  const enter = () => {
+  const handleMouseEnter = () => {
     setOpen(true);
   };
 
-  const leave = () => {
+  const handleMouseLeave = () => {
     setOpen(false);
   };
 
   return (
-    <span onMouseEnter={enter} onMouseLeave={leave}>
+    <div
+      className="af-popover__wrapper"
+      onMouseEnter={handleMouseEnter}
+      onMouseLeave={handleMouseLeave}>
       <PopoverBase
         isOpen={isOpen}
         placement={placement}
         className={className}
         classModifier={classModifier}>
         {children}
       </PopoverBase>
-    </span>
+    </div>
   );
 };
 

diff --git a/packages/popover/src/PopoverBase.js b/packages/popover/src/PopoverBase.js
@@ -74,15 +74,20 @@ export const AnimatedPopover = props => {
   const componentClassName = ClassManager.getComponentClassName(
     className,
     classModifier,
-    defaultClassName,
+    defaultClassName
   );
 
   const [referenceElement, setReferenceElement] = useState(null);
   const [popperElement, setPopperElement] = useState(null);
   const [arrowElement, setArrowElement] = useState(null);
   const { styles, attributes } = usePopper(referenceElement, popperElement, {
     modifiers: [
-      { name: 'arrow', options: { element: arrowElement } },
+      {
+        name: 'arrow',
+        options: {
+          element: arrowElement,
+        },
+      },
       {
         name: 'offset',
         options: {
@@ -111,9 +116,7 @@ export const AnimatedPopover = props => {
           data-popper-placement={placement}
           className="af-popover__container-pop"
           {...attributes.popper}>
-          <div>
-            {children}
-          </div>
+          <div>{children}</div>
           <div
             ref={setArrowElement}
             style={styles.arrow}

diff --git a/packages/popover/src/popover.scss b/packages/popover/src/popover.scss
@@ -1,17 +1,19 @@
 @import '@axa-fr/react-toolkit-core/src/common/scss/core.scss';
 
-$size-arrow: 16px;
+$arrow-size: 16px;
 $arrow-offset: -4px;
 $padding-popover: 0.5rem;
 
 .af-popover {
   &__wrapper {
+    display: inline-block;
     background: inherit;
     border: 0;
     padding: 0;
   }
 
   &__container {
+    cursor: initial;
     display: inline-block;
 
     &-pop {
@@ -43,35 +45,35 @@ $padding-popover: 0.5rem;
     }
 
     .af-popover__arrow {
-      width: $size-arrow;
-      height: $size-arrow;
+      width: $arrow-size;
+      height: $arrow-size;
       position: absolute;
       z-index: -1;
 
       &::before {
         content: '';
         background: $color-azur;
-        width: $size-arrow;
-        height: $size-arrow;
+        width: $arrow-size;
+        height: $arrow-size;
         transform: rotate(45deg);
         position: absolute;
       }
     }
 
     [data-popper-placement^='top'] > .af-popover__arrow {
-      bottom: -4px;
+      bottom: $arrow-offset;
     }
 
     [data-popper-placement^='bottom'] > .af-popover__arrow {
-      top: -4px;
+      top: $arrow-offset;
     }
 
     [data-popper-placement^='right'] > .af-popover__arrow {
-      left: -12px;
+      left: $arrow-offset;
     }
 
     [data-popper-placement^='left'] > .af-popover__arrow {
-      right: 4px;
+      right: $arrow-offset;
     }
   }