فهرست منبع

fix(EditingUserList): refactor AvatarWrapper to use UserPicture component with new props

Yuki Takei 21 ساعت پیش
والد
کامیت
0fe0884616

+ 1 - 4
apps/app/src/client/components/PageEditor/EditorNavbar/EditingUserList.module.scss

@@ -4,7 +4,4 @@
   @extend %user-list-popover;
 }
 
-.avatar-wrapper {
-  // Collapse inline-element ghost space inside the flex container
-  line-height: 0;
-}
+

+ 26 - 14
apps/app/src/client/components/PageEditor/EditorNavbar/EditingUserList.spec.tsx

@@ -18,20 +18,29 @@ import userEvent from '@testing-library/user-event';
 vi.mock('@growi/ui/dist/components', () => ({
   UserPicture: ({
     user,
-    className,
     noTooltip,
+    testId,
+    rootClassName,
+    rootStyle,
+    onClick,
   }: {
     user: EditingClient;
-    className?: string;
     noTooltip?: boolean;
+    testId?: string;
+    rootClassName?: string;
+    rootStyle?: Record<string, string>;
+    onClick?: () => void;
   }) => (
-    <span
-      data-testid={`user-picture-${user.clientId}`}
+    <button
+      type="button"
+      data-testid={testId ?? `user-picture-${user.clientId}`}
       data-no-tooltip={noTooltip ? 'true' : undefined}
-      className={className}
+      className={rootClassName}
+      style={rootStyle}
+      onClick={onClick}
     >
       {user.name}
-    </span>
+    </button>
   ),
 }));
 
@@ -212,15 +221,19 @@ describe('EditingUserList — Task 16.1', () => {
 /**
  * Task 20.4 — EditingUserList tooltip integration
  * Requirements: 7.2
+ *
+ * Tooltip is provided by UserPicture's built-in UncontrolledTooltip.
+ * noTooltip must NOT be passed so the tooltip renders for all avatars.
+ * The testId prop routes the data-testid onto UserPicture's root element,
+ * which is the tooltip target — click and tooltip coexist on the same node.
  */
 describe('EditingUserList — Task 20.4 (tooltip integration)', () => {
   describe('Req 7.2 — UserPicture rendered without noTooltip so tooltip is active', () => {
     it('does not pass noTooltip to UserPicture for direct avatars', () => {
       render(<EditingUserList clientList={[clientAlice]} />);
 
-      const pic = screen.getByTestId('user-picture-1');
-      // data-no-tooltip attribute is only set when noTooltip=true; should be absent
-      expect(pic.getAttribute('data-no-tooltip')).toBeNull();
+      const wrapper = screen.getByTestId('avatar-wrapper-1');
+      expect(wrapper.getAttribute('data-no-tooltip')).toBeNull();
     });
 
     it('does not pass noTooltip to UserPicture for all first-4 direct avatars', () => {
@@ -231,8 +244,8 @@ describe('EditingUserList — Task 20.4 (tooltip integration)', () => {
       );
 
       for (const client of [clientAlice, clientBob, clientCarol, clientDave]) {
-        const pic = screen.getByTestId(`user-picture-${client.clientId}`);
-        expect(pic.getAttribute('data-no-tooltip')).toBeNull();
+        const wrapper = screen.getByTestId(`avatar-wrapper-${client.clientId}`);
+        expect(wrapper.getAttribute('data-no-tooltip')).toBeNull();
       }
     });
 
@@ -249,11 +262,10 @@ describe('EditingUserList — Task 20.4 (tooltip integration)', () => {
         />,
       );
 
-      // Open the overflow popover
       await userEvent.click(screen.getByRole('button', { name: /^\+1$/ }));
 
-      const evePic = screen.getByTestId('user-picture-5');
-      expect(evePic.getAttribute('data-no-tooltip')).toBeNull();
+      const eveWrapper = screen.getByTestId('avatar-wrapper-5');
+      expect(eveWrapper.getAttribute('data-no-tooltip')).toBeNull();
     });
   });
 });

+ 49 - 51
apps/app/src/client/components/PageEditor/EditorNavbar/EditingUserList.tsx

@@ -6,7 +6,6 @@ import { Popover, PopoverBody } from 'reactstrap';
 import styles from './EditingUserList.module.scss';
 
 const userListPopoverClass = styles['user-list-popover'] ?? '';
-const avatarWrapperClass = styles['avatar-wrapper'] ?? '';
 
 type Props = {
   clientList: EditingClient[];
@@ -18,15 +17,16 @@ const AvatarWrapper: FC<{
   onUserClick?: (clientId: number) => void;
 }> = ({ client, onUserClick }) => {
   return (
-    <button
-      type="button"
-      data-testid={`avatar-wrapper-${client.clientId}`}
-      className={`${avatarWrapperClass} d-inline-flex align-items-center justify-content-center p-0 bg-transparent rounded-circle`}
-      style={{ border: `2px solid ${client.color}` }}
-      onClick={() => onUserClick?.(client.clientId)}
-    >
-      <UserPicture user={client} noLink />
-    </button>
+    <UserPicture
+      user={client}
+      noLink
+      testId={`avatar-wrapper-${client.clientId}`}
+      rootClassName="d-flex rounded-circle"
+      rootStyle={{ border: `2px solid ${client.color}` }}
+      onClick={
+        onUserClick != null ? () => onUserClick(client.clientId) : undefined
+      }
+    />
   );
 };
 
@@ -44,48 +44,46 @@ export const EditingUserList: FC<Props> = ({ clientList, onUserClick }) => {
   }
 
   return (
-    <div className="d-flex flex-column justify-content-start justify-content-sm-end">
-      <div className="d-flex justify-content-start justify-content-sm-end">
-        {firstFourUsers.map((editingClient) => (
-          <div key={editingClient.clientId} className="ms-1">
-            <AvatarWrapper client={editingClient} onUserClick={onUserClick} />
-          </div>
-        ))}
+    <div className="d-flex">
+      {firstFourUsers.map((editingClient) => (
+        <div key={editingClient.clientId} className="ms-1">
+          <AvatarWrapper client={editingClient} onUserClick={onUserClick} />
+        </div>
+      ))}
 
-        {remainingUsers.length > 0 && (
-          <div className="ms-1">
-            <button
-              type="button"
-              ref={popoverTargetRef}
-              className="btn border-0 bg-info-subtle rounded-pill p-0"
-              onClick={togglePopover}
-            >
-              <span className="fw-bold text-info p-1">
-                +{remainingUsers.length}
-              </span>
-            </button>
-            <Popover
-              placement="bottom"
-              isOpen={isPopoverOpen}
-              target={popoverTargetRef}
-              toggle={togglePopover}
-              trigger="legacy"
-            >
-              <PopoverBody className={userListPopoverClass}>
-                <div className="d-flex flex-wrap gap-1">
-                  {remainingUsers.map((editingClient) => (
-                    <AvatarWrapper
-                      key={editingClient.clientId}
-                      client={editingClient}
-                      onUserClick={onUserClick}
-                    />
-                  ))}
-                </div>
-              </PopoverBody>
-            </Popover>
-          </div>
-        )}
-      </div>
+      {remainingUsers.length > 0 && (
+        <div className="ms-1">
+          <button
+            type="button"
+            ref={popoverTargetRef}
+            className="btn border-0 bg-info-subtle rounded-pill p-0"
+            onClick={togglePopover}
+          >
+            <span className="fw-bold text-info p-1">
+              +{remainingUsers.length}
+            </span>
+          </button>
+          <Popover
+            placement="bottom"
+            isOpen={isPopoverOpen}
+            target={popoverTargetRef}
+            toggle={togglePopover}
+            trigger="legacy"
+          >
+            <PopoverBody className={userListPopoverClass}>
+              <div className="d-flex flex-wrap gap-1">
+                {remainingUsers.map((editingClient) => (
+                  <AvatarWrapper
+                    key={editingClient.clientId}
+                    client={editingClient}
+                    onUserClick={onUserClick}
+                  />
+                ))}
+              </div>
+            </PopoverBody>
+          </Popover>
+        </div>
+      )}
     </div>
   );
 };

+ 49 - 2
packages/ui/src/components/UserPicture.tsx

@@ -1,4 +1,5 @@
 import {
+  type CSSProperties,
   forwardRef,
   type JSX,
   memo,
@@ -33,7 +34,12 @@ type BaseUserPictureRootProps = {
   className?: string;
 };
 
-type UserPictureRootWithoutLinkProps = BaseUserPictureRootProps;
+type UserPictureRootWithoutLinkProps = BaseUserPictureRootProps & {
+  onClick?: () => void;
+  rootClassName?: string;
+  rootStyle?: CSSProperties;
+  testId?: string;
+};
 
 type UserPictureRootWithLinkProps = BaseUserPictureRootProps & {
   username: string;
@@ -43,8 +49,37 @@ const UserPictureRootWithoutLink = forwardRef<
   HTMLSpanElement,
   UserPictureRootWithoutLinkProps
 >((props, ref) => {
+  const { onClick, rootClassName, rootStyle, testId } = props;
+  const interactive = onClick != null;
+  const resolvedStyle: CSSProperties | undefined = interactive
+    ? { cursor: 'pointer', ...rootStyle }
+    : rootStyle;
+  if (interactive) {
+    return (
+      // biome-ignore lint/a11y/useSemanticElements: UserPicture is used in varied layout contexts where a native button would break styling
+      <span
+        ref={ref}
+        className={rootClassName ?? props.className}
+        style={resolvedStyle}
+        data-testid={testId}
+        onClick={onClick}
+        onKeyDown={(e) => {
+          if (e.key === 'Enter' || e.key === ' ') onClick();
+        }}
+        role="button"
+        tabIndex={0}
+      >
+        {props.children}
+      </span>
+    );
+  }
   return (
-    <span ref={ref} className={props.className}>
+    <span
+      ref={ref}
+      className={rootClassName ?? props.className}
+      style={resolvedStyle}
+      data-testid={testId}
+    >
       {props.children}
     </span>
   );
@@ -115,6 +150,10 @@ type Props = {
   noLink?: boolean;
   noTooltip?: boolean;
   className?: string;
+  onClick?: () => void;
+  rootClassName?: string;
+  rootStyle?: CSSProperties;
+  testId?: string;
 };
 
 export const UserPicture = memo((userProps: Props): JSX.Element => {
@@ -124,6 +163,10 @@ export const UserPicture = memo((userProps: Props): JSX.Element => {
     noLink,
     noTooltip,
     className: additionalClassName,
+    onClick,
+    rootClassName,
+    rootStyle,
+    testId,
   } = userProps;
 
   // Extract user information
@@ -183,6 +226,10 @@ export const UserPicture = memo((userProps: Props): JSX.Element => {
         ref={rootRef}
         displayName={displayName}
         size={size}
+        onClick={onClick}
+        rootClassName={rootClassName}
+        rootStyle={rootStyle}
+        testId={testId}
       >
         {children}
       </UserPictureRootWithoutLink>