Browse Source

ref(hovercard): refactor to fc and use react testing library (#31394)

* ref(hovercard): refactor to fc

* test(hovercard): add tests

* fix(stories): update name

* fix(hovercard): imports

* fix(hovercard): hide on prop change should work

* test(hovercard): change export

* fix(commitrow): update spec

* fix(hovercard): code review comments

* ref(hovercard): parse transform

* ref(hovercard): add wrapper style so we can animate transfrom instead of top/left

* ref(hovercard): remove useMemo

* ref(hovercard): lint alphabetic sort

* style(lint): Auto commit lint changes

* style(lint): Auto commit lint changes

* test(hovercard): use await for final check

* fix(hovercard): fix z index

* empty commit

Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
Jonas 3 years ago
parent
commit
eeceee591c

+ 6 - 2
docs-ui/stories/components/hovercard.stories.js

@@ -1,4 +1,4 @@
-import Hovercard from 'sentry/components/hovercard';
+import {Hovercard} from 'sentry/components/hovercard';
 
 export default {
   title: 'Components/Tooltips/Hovercard',
@@ -7,10 +7,13 @@ export default {
     header: 'Header',
     body: 'Body',
     position: 'top',
-    show: true,
   },
   argTypes: {
     tipColor: {control: 'color'},
+    show: {
+      type: 'select',
+      options: [undefined, false, true],
+    },
   },
 };
 
@@ -26,6 +29,7 @@ export const _Hovercard = ({...args}) => (
     <Hovercard {...args}>Hover over me</Hovercard>
   </div>
 );
+
 _Hovercard.parameters = {
   docs: {
     description: {

+ 1 - 1
static/app/components/assistant/guideAnchor.tsx

@@ -13,7 +13,7 @@ import {
 } from 'sentry/actionCreators/guides';
 import {Guide} from 'sentry/components/assistant/types';
 import Button from 'sentry/components/button';
-import Hovercard, {Body as HovercardBody} from 'sentry/components/hovercard';
+import {Body as HovercardBody, Hovercard} from 'sentry/components/hovercard';
 import {t, tct} from 'sentry/locale';
 import GuideStore, {GuideStoreState} from 'sentry/stores/guideStore';
 import space from 'sentry/styles/space';

+ 1 - 1
static/app/components/commitRow.tsx

@@ -5,7 +5,7 @@ import * as Sentry from '@sentry/react';
 import {openInviteMembersModal} from 'sentry/actionCreators/modal';
 import UserAvatar from 'sentry/components/avatar/userAvatar';
 import CommitLink from 'sentry/components/commitLink';
-import Hovercard from 'sentry/components/hovercard';
+import {Hovercard} from 'sentry/components/hovercard';
 import Link from 'sentry/components/links/link';
 import {PanelItem} from 'sentry/components/panels';
 import TextOverflow from 'sentry/components/textOverflow';

+ 1 - 1
static/app/components/discover/discoverFeature.tsx

@@ -2,7 +2,7 @@ import * as React from 'react';
 
 import Feature from 'sentry/components/acl/feature';
 import FeatureDisabled from 'sentry/components/acl/featureDisabled';
-import Hovercard from 'sentry/components/hovercard';
+import {Hovercard} from 'sentry/components/hovercard';
 import {t} from 'sentry/locale';
 
 type Props = {

+ 1 - 1
static/app/components/events/interfaces/crashContent/exception/mechanism.tsx

@@ -5,7 +5,7 @@ import forOwn from 'lodash/forOwn';
 import isNil from 'lodash/isNil';
 import isObject from 'lodash/isObject';
 
-import Hovercard from 'sentry/components/hovercard';
+import {Hovercard} from 'sentry/components/hovercard';
 import ExternalLink from 'sentry/components/links/externalLink';
 import Pill from 'sentry/components/pill';
 import Pills from 'sentry/components/pills';

+ 7 - 1
static/app/components/group/suggestedOwnerHovercard.tsx

@@ -6,7 +6,7 @@ import {openInviteMembersModal} from 'sentry/actionCreators/modal';
 import Alert from 'sentry/components/alert';
 import ActorAvatar from 'sentry/components/avatar/actorAvatar';
 import Button from 'sentry/components/button';
-import Hovercard from 'sentry/components/hovercard';
+import {Hovercard} from 'sentry/components/hovercard';
 import Link from 'sentry/components/links/link';
 import {IconCommit, IconWarning} from 'sentry/icons';
 import {t, tct} from 'sentry/locale';
@@ -20,11 +20,17 @@ type Props = {
    * The suggested actor.
    */
   actor: Actor;
+  /**
+   * Children are required, as they are passed to the hovercard component, without it,
+   * we will not be able to trigger any hovercard actions
+   */
+  children: React.ReactNode;
   /**
    * The list of commits the actor is suggested for. May be left blank if the
    * actor is not suggested for commits.
    */
   commits?: Commit[];
+
   /**
    * The list of ownership rules the actor is suggested for. May be left blank
    * if the actor is not suggested based on ownership rules.

+ 1 - 1
static/app/components/group/suggestedOwners/ownershipRules.tsx

@@ -8,7 +8,7 @@ import GuideAnchor from 'sentry/components/assistant/guideAnchor';
 import Button from 'sentry/components/button';
 import ButtonBar from 'sentry/components/buttonBar';
 import FeatureBadge from 'sentry/components/featureBadge';
-import Hovercard from 'sentry/components/hovercard';
+import {Hovercard} from 'sentry/components/hovercard';
 import {Panel} from 'sentry/components/panels';
 import {IconClose, IconQuestion} from 'sentry/icons';
 import {t} from 'sentry/locale';

+ 222 - 189
static/app/components/hovercard.tsx

@@ -1,30 +1,34 @@
 import * as React from 'react';
 import ReactDOM from 'react-dom';
 import {Manager, Popper, PopperProps, Reference} from 'react-popper';
-import {keyframes} from '@emotion/react';
 import styled from '@emotion/styled';
 import classNames from 'classnames';
+import {motion} from 'framer-motion';
 
-import {fadeIn} from 'sentry/styles/animations';
 import space from 'sentry/styles/space';
 import {domId} from 'sentry/utils/domId';
 
-const VALID_DIRECTIONS = ['top', 'bottom', 'left', 'right'] as const;
+export const HOVERCARD_PORTAL_ID = 'hovercard-portal';
 
-type Direction = typeof VALID_DIRECTIONS[number];
+function findOrCreatePortal(): HTMLElement {
+  let portal = document.getElementById(HOVERCARD_PORTAL_ID);
 
-type DefaultProps = {
-  /**
-   * Time in ms until hovercard is hidden
-   */
-  displayTimeout: number;
+  if (portal) {
+    return portal;
+  }
+
+  portal = document.createElement('div');
+  portal.setAttribute('id', HOVERCARD_PORTAL_ID);
+  document.body.appendChild(portal);
+
+  return portal;
+}
+
+interface HovercardProps {
   /**
-   * Position tooltip should take relative to the child element
+   * Classname to apply to the hovercard
    */
-  position: Direction;
-};
-
-type Props = DefaultProps & {
+  children: React.ReactNode;
   /**
    * Element to display in the body
    */
@@ -33,14 +37,15 @@ type Props = DefaultProps & {
    * Classname to apply to body container
    */
   bodyClassName?: string;
-  /**
-   * Classname to apply to the hovercard
-   */
   className?: string;
   /**
    * Classname to apply to the hovercard container
    */
   containerClassName?: string;
+  /**
+   * Time in ms until hovercard is hidden
+   */
+  displayTimeout?: number;
   /**
    * Element to display in the header
    */
@@ -53,6 +58,10 @@ type Props = DefaultProps & {
    * Offset for the arrow
    */
   offset?: string;
+  /**
+   * Position tooltip should take relative to the child element
+   */
+  position?: PopperProps['placement'];
   /**
    * If set, is used INSTEAD OF the hover action to determine whether the hovercard is shown
    */
@@ -65,82 +74,43 @@ type Props = DefaultProps & {
    * Color of the arrow tip
    */
   tipColor?: string;
-};
-
-type State = {
-  visible: boolean;
-};
-
-class Hovercard extends React.Component<Props, State> {
-  static defaultProps: DefaultProps = {
-    displayTimeout: 100,
-    position: 'top',
-  };
-
-  constructor(args: Props) {
-    super(args);
-
-    let portal = document.getElementById('hovercard-portal');
-    if (!portal) {
-      portal = document.createElement('div');
-      portal.setAttribute('id', 'hovercard-portal');
-      document.body.appendChild(portal);
-    }
-    this.portalEl = portal;
-    this.tooltipId = domId('hovercard-');
-    this.scheduleUpdate = null;
-  }
-
-  state: State = {
-    visible: false,
-  };
-
-  componentDidUpdate(prevProps: Props) {
-    const {body, header} = this.props;
-
-    if (body !== prevProps.body || header !== prevProps.header) {
-      // We had a problem with popper not recalculating position when body/header changed while hovercard still opened.
-      // This can happen for example when showing a loading spinner in a hovercard and then changing it to the actual content once fetch finishes.
-      this.scheduleUpdate?.();
-    }
-  }
+}
 
-  portalEl: HTMLElement;
-  tooltipId: string;
-  hoverWait: number | null = null;
-  scheduleUpdate: (() => void) | null;
+function Hovercard(props: HovercardProps): React.ReactElement {
+  const [visible, setVisible] = React.useState(false);
 
-  handleToggleOn = () => this.toggleHovercard(true);
-  handleToggleOff = () => this.toggleHovercard(false);
+  const inTimeout = React.useRef<number | null>(null);
+  const scheduleUpdateRef = React.useRef<(() => void) | null>(null);
 
-  toggleHovercard = (visible: boolean) => {
-    const {displayTimeout} = this.props;
+  const portalEl = React.useMemo(() => findOrCreatePortal(), []);
+  const tooltipId = React.useMemo(() => domId('hovercard-'), []);
 
-    if (this.hoverWait) {
-      clearTimeout(this.hoverWait);
+  React.useEffect(() => {
+    // We had a problem with popper not recalculating position when body/header changed while hovercard still opened.
+    // This can happen for example when showing a loading spinner in a hovercard and then changing it to the actual content once fetch finishes.
+    if (scheduleUpdateRef.current) {
+      scheduleUpdateRef.current();
     }
-
-    this.hoverWait = window.setTimeout(() => this.setState({visible}), displayTimeout);
-  };
-
-  render() {
-    const {
-      bodyClassName,
-      containerClassName,
-      className,
-      header,
-      body,
-      position,
-      show,
-      tipColor,
-      tipBorderColor,
-      offset,
-      modifiers,
-    } = this.props;
-
-    // Maintain the hovercard class name for BC with less styles
-    const cx = classNames('hovercard', className);
-    const popperModifiers: PopperProps['modifiers'] = {
+  }, [props.body, props.header]);
+
+  const toggleHovercard = React.useCallback(
+    (value: boolean) => {
+      // If a previous timeout is set, then clear it
+      if (typeof inTimeout.current === 'number') {
+        clearTimeout(inTimeout.current);
+      }
+
+      // Else enqueue a new timeout
+      inTimeout.current = window.setTimeout(
+        () => setVisible(value),
+        props.displayTimeout ?? 100
+      );
+    },
+    [props.displayTimeout]
+  );
+
+  const popperModifiers = React.useMemo(() => {
+    const modifiers: PopperProps['modifiers'] = {
       hide: {
         enabled: false,
       },
@@ -149,91 +119,163 @@ class Hovercard extends React.Component<Props, State> {
         enabled: true,
         boundariesElement: 'viewport',
       },
-      ...(modifiers || {}),
+      ...(props.modifiers || {}),
     };
-
-    const visible = show !== undefined ? show : this.state.visible;
-    const hoverProps =
-      show !== undefined
-        ? {}
-        : {onMouseEnter: this.handleToggleOn, onMouseLeave: this.handleToggleOff};
-
-    return (
-      <Manager>
-        <Reference>
-          {({ref}) => (
-            <span
-              ref={ref}
-              aria-describedby={this.tooltipId}
-              className={containerClassName}
-              {...hoverProps}
-            >
-              {this.props.children}
-            </span>
-          )}
-        </Reference>
-        {visible &&
-          (header || body) &&
-          ReactDOM.createPortal(
-            <Popper placement={position} modifiers={popperModifiers}>
-              {({ref, style, placement, arrowProps, scheduleUpdate}) => {
-                this.scheduleUpdate = scheduleUpdate;
-                return (
+    return modifiers;
+  }, [props.modifiers]);
+
+  // If show is not set, then visibility state is uncontrolled
+  const isVisible = props.show === undefined ? visible : props.show;
+
+  const hoverProps = React.useMemo((): Pick<
+    React.HTMLProps<HTMLDivElement>,
+    'onMouseEnter' | 'onMouseLeave'
+  > => {
+    // If show is not set, then visibility state is controlled by mouse events
+    if (props.show === undefined) {
+      return {
+        onMouseEnter: () => toggleHovercard(true),
+        onMouseLeave: () => toggleHovercard(false),
+      };
+    }
+    return {};
+  }, [props.show, toggleHovercard]);
+
+  return (
+    <Manager>
+      <Reference>
+        {({ref}) => (
+          <span
+            ref={ref}
+            aria-describedby={tooltipId}
+            className={props.containerClassName}
+            {...hoverProps}
+          >
+            {props.children}
+          </span>
+        )}
+      </Reference>
+      {ReactDOM.createPortal(
+        <Popper placement={props.position ?? 'top'} modifiers={popperModifiers}>
+          {({ref, style, placement, arrowProps, scheduleUpdate}) => {
+            scheduleUpdateRef.current = scheduleUpdate;
+
+            // Element is not visible in neither controlled and uncontrolled state (show prop is not passed and card is not hovered)
+            if (!isVisible) {
+              return null;
+            }
+
+            // Nothing to render
+            if (!props.body && !props.header) {
+              return null;
+            }
+
+            return (
+              <HovercardContainer style={style}>
+                <SlideInAnimation visible={isVisible} placement={placement}>
                   <StyledHovercard
-                    id={this.tooltipId}
-                    visible={visible}
                     ref={ref}
-                    style={style}
-                    placement={placement as Direction}
-                    offset={offset}
-                    className={cx}
+                    id={tooltipId}
+                    placement={placement}
+                    offset={props.offset}
+                    // Maintain the hovercard class name for BC with less styles
+                    className={classNames('hovercard', props.className)}
                     {...hoverProps}
                   >
-                    {header && <Header>{header}</Header>}
-                    {body && <Body className={bodyClassName}>{body}</Body>}
+                    {props.header ? <Header>{props.header}</Header> : null}
+                    {props.body ? (
+                      <Body className={props.bodyClassName}>{props.body}</Body>
+                    ) : null}
                     <HovercardArrow
                       ref={arrowProps.ref}
                       style={arrowProps.style}
-                      placement={placement as Direction}
-                      tipColor={tipColor}
-                      tipBorderColor={tipBorderColor}
+                      placement={placement}
+                      tipColor={props.tipColor}
+                      tipBorderColor={props.tipBorderColor}
                     />
                   </StyledHovercard>
-                );
-              }}
-            </Popper>,
-            this.portalEl
-          )}
-      </Manager>
-    );
-  }
+                </SlideInAnimation>
+              </HovercardContainer>
+            );
+          }}
+        </Popper>,
+        portalEl
+      )}
+    </Manager>
+  );
 }
 
-// Slide in from the same direction as the placement
-// so that the card pops into place.
-const slideIn = (p: StyledHovercardProps) => keyframes`
-  from {
-    ${p.placement === 'top' ? 'top: -10px;' : ''}
-    ${p.placement === 'bottom' ? 'top: 10px;' : ''}
-    ${p.placement === 'left' ? 'left: -10px;' : ''}
-    ${p.placement === 'right' ? 'left: 10px;' : ''}
-  }
-  to {
-    ${p.placement === 'top' ? 'top: 0;' : ''}
-    ${p.placement === 'bottom' ? 'top: 0;' : ''}
-    ${p.placement === 'left' ? 'left: 0;' : ''}
-    ${p.placement === 'right' ? 'left: 0;' : ''}
+export {Hovercard};
+
+const SLIDE_DISTANCE = 10;
+
+function SlideInAnimation({
+  visible,
+  placement,
+  children,
+}: {
+  children: React.ReactNode;
+  placement: PopperProps['placement'];
+  visible: boolean;
+}): React.ReactElement {
+  const narrowedPlacement = getTipDirection(placement);
+
+  const x =
+    narrowedPlacement === 'left'
+      ? [-SLIDE_DISTANCE, 0]
+      : narrowedPlacement === 'right'
+      ? [SLIDE_DISTANCE, 0]
+      : [0, 0];
+
+  const y =
+    narrowedPlacement === 'top'
+      ? [-SLIDE_DISTANCE, 0]
+      : narrowedPlacement === 'bottom'
+      ? [SLIDE_DISTANCE, 0]
+      : [0, 0];
+
+  return (
+    <motion.div
+      initial="hidden"
+      variants={{
+        hidden: {
+          opacity: 0,
+        },
+        visible: {
+          opacity: [0, 1],
+          x,
+          y,
+        },
+      }}
+      animate={visible ? 'visible' : 'hidden'}
+      transition={{duration: 0.1, ease: 'easeInOut'}}
+    >
+      {children}
+    </motion.div>
+  );
+}
+
+function getTipDirection(
+  placement: HovercardArrowProps['placement']
+): 'top' | 'bottom' | 'left' | 'right' {
+  if (!placement) {
+    return 'top';
   }
-`;
 
-const getTipDirection = (p: HovercardArrowProps) =>
-  VALID_DIRECTIONS.includes(p.placement) ? p.placement : 'top';
+  const prefix = ['top', 'bottom', 'left', 'right'].find(pl => {
+    return placement.startsWith(pl);
+  });
+
+  return (prefix || 'top') as 'top' | 'bottom' | 'left' | 'right';
+}
 
-const getOffset = (p: StyledHovercardProps) => p.offset ?? space(2);
+const HovercardContainer = styled('div')`
+  /* Some hovercards overlap the toplevel header and sidebar, and we need to appear on top */
+  z-index: ${p => p.theme.zIndex.hovercard};
+`;
 
 type StyledHovercardProps = {
-  placement: Direction;
-  visible: boolean;
+  placement: PopperProps['placement'];
   offset?: string;
 };
 
@@ -242,8 +284,6 @@ const StyledHovercard = styled('div')<StyledHovercardProps>`
   text-align: left;
   padding: 0;
   line-height: 1;
-  /* Some hovercards overlap the toplevel header and sidebar, and we need to appear on top */
-  z-index: ${p => p.theme.zIndex.hovercard};
   white-space: initial;
   color: ${p => p.theme.textColor};
   border: 1px solid ${p => p.theme.border};
@@ -255,17 +295,11 @@ const StyledHovercard = styled('div')<StyledHovercardProps>`
   /* The hovercard may appear in different contexts, don't inherit fonts */
   font-family: ${p => p.theme.text.family};
 
-  position: absolute;
-  visibility: ${p => (p.visible ? 'visible' : 'hidden')};
-
-  animation: ${fadeIn} 100ms, ${slideIn} 100ms ease-in-out;
-  animation-play-state: ${p => (p.visible ? 'running' : 'paused')};
-
   /* Offset for the arrow */
-  ${p => (p.placement === 'top' ? `margin-bottom: ${getOffset(p)}` : '')};
-  ${p => (p.placement === 'bottom' ? `margin-top: ${getOffset(p)}` : '')};
-  ${p => (p.placement === 'left' ? `margin-right: ${getOffset(p)}` : '')};
-  ${p => (p.placement === 'right' ? `margin-left: ${getOffset(p)}` : '')};
+  ${p => (p.placement === 'top' ? `margin-bottom: ${p.offset ?? space(2)}` : '')};
+  ${p => (p.placement === 'bottom' ? `margin-top: ${p.offset ?? space(2)}` : '')};
+  ${p => (p.placement === 'left' ? `margin-right: ${p.offset ?? space(2)}` : '')};
+  ${p => (p.placement === 'right' ? `margin-left: ${p.offset ?? space(2)}` : '')};
 `;
 
 const Header = styled('div')`
@@ -278,13 +312,17 @@ const Header = styled('div')`
   padding: ${space(1.5)};
 `;
 
+export {Header};
+
 const Body = styled('div')`
   padding: ${space(2)};
   min-height: 30px;
 `;
 
+export {Body};
+
 type HovercardArrowProps = {
-  placement: Direction;
+  placement: PopperProps['placement'];
   tipBorderColor?: string;
   tipColor?: string;
 };
@@ -293,12 +331,10 @@ const HovercardArrow = styled('span')<HovercardArrowProps>`
   position: absolute;
   width: 20px;
   height: 20px;
-  z-index: -1;
-
-  ${p => (p.placement === 'top' ? 'bottom: -20px; left: 0' : '')};
-  ${p => (p.placement === 'bottom' ? 'top: -20px; left: 0' : '')};
-  ${p => (p.placement === 'left' ? 'right: -20px' : '')};
-  ${p => (p.placement === 'right' ? 'left: -20px' : '')};
+  right: ${p => (p.placement === 'left' ? '-3px' : 'auto')};
+  left: ${p => (p.placement === 'right' ? '-3px' : 'auto')};
+  bottom: ${p => (p.placement === 'top' ? '-3px' : 'auto')};
+  top: ${p => (p.placement === 'bottom' ? '-3px' : 'auto')};
 
   &::before,
   &::after {
@@ -316,18 +352,15 @@ const HovercardArrow = styled('span')<HovercardArrowProps>`
   &::before {
     top: 1px;
     border: 10px solid transparent;
-    border-${getTipDirection}-color: ${p =>
-  p.tipBorderColor || p.tipColor || p.theme.border};
-
-    ${p => (p.placement === 'bottom' ? 'top: -1px' : '')};
-    ${p => (p.placement === 'left' ? 'top: 0; left: 1px;' : '')};
-    ${p => (p.placement === 'right' ? 'top: 0; left: -1px' : '')};
-  }
-  &::after {
-    border: 10px solid transparent;
-    border-${getTipDirection}-color: ${p => p.tipColor ?? p.theme.background};
-  }
+    border-${p => getTipDirection(p.placement)}-color:
+      ${p => p.tipBorderColor || p.tipColor || p.theme.border};
+      ${p => (p.placement === 'bottom' ? 'top: -1px' : '')};
+      ${p => (p.placement === 'left' ? 'top: 0; left: 1px;' : '')};
+      ${p => (p.placement === 'right' ? 'top: 0; left: -1px' : '')};
+    }
+    &::after {
+      border: 10px solid transparent;
+      border-${p => getTipDirection(p.placement)}-color: ${p =>
+  p.tipColor ?? p.theme.background};
+    }
 `;
-
-export {Body, Header, Hovercard};
-export default Hovercard;

+ 1 - 1
static/app/components/issueLink.tsx

@@ -6,7 +6,7 @@ import Count from 'sentry/components/count';
 import EventOrGroupTitle from 'sentry/components/eventOrGroupTitle';
 import EventAnnotation from 'sentry/components/events/eventAnnotation';
 import EventMessage from 'sentry/components/events/eventMessage';
-import Hovercard from 'sentry/components/hovercard';
+import {Hovercard} from 'sentry/components/hovercard';
 import Link from 'sentry/components/links/link';
 import TimeSince from 'sentry/components/timeSince';
 import {t} from 'sentry/locale';

+ 1 - 13
static/app/components/issueSyncListElement.tsx

@@ -3,7 +3,7 @@ import {ClassNames} from '@emotion/react';
 import styled from '@emotion/styled';
 import capitalize from 'lodash/capitalize';
 
-import Hovercard from 'sentry/components/hovercard';
+import {Hovercard} from 'sentry/components/hovercard';
 import {IconAdd, IconClose} from 'sentry/icons';
 import space from 'sentry/styles/space';
 import {callIfFunction} from 'sentry/utils/callIfFunction';
@@ -23,17 +23,6 @@ type Props = {
 };
 
 class IssueSyncListElement extends React.Component<Props> {
-  componentDidUpdate(nextProps) {
-    if (
-      this.props.showHoverCard !== nextProps.showHoverCard &&
-      nextProps.showHoverCard === undefined
-    ) {
-      this.hoverCardRef.current && this.hoverCardRef.current.handleToggleOff();
-    }
-  }
-
-  hoverCardRef = React.createRef<Hovercard>();
-
   isLinked(): boolean {
     return !!(this.props.externalIssueLink && this.props.externalIssueId);
   }
@@ -103,7 +92,6 @@ class IssueSyncListElement extends React.Component<Props> {
         <ClassNames>
           {({css}) => (
             <Hovercard
-              ref={this.hoverCardRef}
               containerClassName={css`
                 display: flex;
                 align-items: center;

Some files were not shown because too many files changed in this diff