From a2289471a3d678260d8e28f1a3c60f213946bfa8 Mon Sep 17 00:00:00 2001
From: Robert Knight <robertknight@gmail.com>
Date: Wed, 24 Jan 2024 10:54:01 +0000
Subject: [PATCH] Fix top bar toggle buttons extending outside top bar on touch
 devices

There was already a solution for this issue implemented in `SearchIconButton`.
Extract that into a component that the other top bar buttons can use.

Part of https://github.com/hypothesis/client/issues/6131.
---
 src/sidebar/components/TopBar.tsx             | 27 ++++++++++++++++---
 .../components/search/SearchIconButton.tsx    | 12 ++-------
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/src/sidebar/components/TopBar.tsx b/src/sidebar/components/TopBar.tsx
index 077dff1034a..d78290fb373 100644
--- a/src/sidebar/components/TopBar.tsx
+++ b/src/sidebar/components/TopBar.tsx
@@ -1,3 +1,4 @@
+import type { IconButtonProps } from '@hypothesis/frontend-shared';
 import { LinkButton, HelpIcon, ShareIcon } from '@hypothesis/frontend-shared';
 import classnames from 'classnames';
 
@@ -34,6 +35,26 @@ export type TopBarProps = {
   settings: SidebarSettings;
 };
 
+/**
+ * Toggle button for the top bar, with a background to indicate its "pressed"
+ * state.
+ */
+export function TopBarToggleButton(buttonProps: IconButtonProps) {
+  return (
+    <PressableIconButton
+      // The containing form has a white background. The top bar is only
+      // 40px high. If we allow standard touch-minimum height here (44px),
+      // the visible white background exceeds the height of the top bar in
+      // touch contexts. Disable touch sizing via `size="custom"`, then
+      // add back the width rule and padding to keep horizontal spacing
+      // consistent.
+      size="custom"
+      classes="touch:min-w-touch-minimum p-1"
+      {...buttonProps}
+    />
+  );
+}
+
 /**
  * The toolbar which appears at the top of the sidebar providing actions
  * to switch groups, view account information, sort/filter annotations etc.
@@ -105,23 +126,21 @@ function TopBar({
               )}
               {searchPanelEnabled && <SearchIconButton />}
               <SortMenu />
-              <PressableIconButton
+              <TopBarToggleButton
                 icon={ShareIcon}
                 expanded={isAnnotationsPanelOpen}
                 pressed={isAnnotationsPanelOpen}
                 onClick={toggleSharePanel}
-                size="xs"
                 title="Share annotations on this page"
                 data-testid="share-icon-button"
               />
             </>
           )}
-          <PressableIconButton
+          <TopBarToggleButton
             icon={HelpIcon}
             expanded={isHelpPanelOpen}
             pressed={isHelpPanelOpen}
             onClick={requestHelp}
-            size="xs"
             title="Help"
             data-testid="help-icon-button"
           />
diff --git a/src/sidebar/components/search/SearchIconButton.tsx b/src/sidebar/components/search/SearchIconButton.tsx
index 7d4499b6653..92a4d1229cc 100644
--- a/src/sidebar/components/search/SearchIconButton.tsx
+++ b/src/sidebar/components/search/SearchIconButton.tsx
@@ -5,7 +5,7 @@ import { useShortcut } from '../../../shared/shortcut';
 import { isMacOS } from '../../../shared/user-agent';
 import type { SidebarStore } from '../../store';
 import { useSidebarStore } from '../../store';
-import PressableIconButton from '../PressableIconButton';
+import { TopBarToggleButton } from '../TopBar';
 
 /**
  * Respond to keydown events on the document (shortcut keys):
@@ -61,20 +61,12 @@ export default function SearchIconButton() {
     <>
       {isLoading && <Spinner />}
       {!isLoading && (
-        <PressableIconButton
+        <TopBarToggleButton
           icon={SearchIcon}
           expanded={isSearchPanelOpen}
           pressed={isSearchPanelOpen}
           onClick={toggleSearchPanel}
           title="Search annotations"
-          // The containing form has a white background. The top bar is only
-          // 40px high. If we allow standard touch-minimum height here (44px),
-          // the visible white background exceeds the height of the top bar in
-          // touch contexts. Disable touch sizing via `size="custom"`, then
-          // add back the width rule and padding to keep horizontal spacing
-          // consistent.
-          size="custom"
-          classes="touch:min-w-touch-minimum p-1"
         />
       )}
     </>