From 59493c94cba102fddd6248eeb39abe9ee95f069f Mon Sep 17 00:00:00 2001
From: somebody1234 <ehern.lee@gmail.com>
Date: Thu, 21 Nov 2024 06:43:49 +1000
Subject: [PATCH] Fix keyboard shortcuts settings display (#11467)

- Add border to keyboard shortcuts to make them more distinct from each other
- Move scrollbar to table so the header becomes sticky

# Important Notes
None
---
 .../components/dashboard/KeyboardShortcut.tsx |  7 +++-
 .../KeyboardShortcutsSettingsSection.tsx      | 33 ++++++++-----------
 .../layouts/Settings/MembersTable.tsx         |  2 +-
 .../dashboard/layouts/Settings/Section.tsx    |  2 +-
 .../src/dashboard/layouts/Settings/Tab.tsx    |  5 ++-
 .../src/dashboard/layouts/Settings/data.tsx   |  1 +
 app/gui/src/dashboard/tailwind.css            |  8 -----
 app/gui/tailwind.config.js                    |  7 ----
 8 files changed, 26 insertions(+), 39 deletions(-)

diff --git a/app/gui/src/dashboard/components/dashboard/KeyboardShortcut.tsx b/app/gui/src/dashboard/components/dashboard/KeyboardShortcut.tsx
index 807614828cf..264bb1ef6a7 100644
--- a/app/gui/src/dashboard/components/dashboard/KeyboardShortcut.tsx
+++ b/app/gui/src/dashboard/components/dashboard/KeyboardShortcut.tsx
@@ -100,11 +100,13 @@ const MODIFIER_TO_TEXT_ID: Readonly<Record<ModifierKey, text.TextId>> = {
 /** Props for a {@link KeyboardShortcut}, specifying the keyboard action. */
 export interface KeyboardShortcutActionProps {
   readonly action: DashboardBindingKey
+  readonly className?: string
 }
 
 /** Props for a {@link KeyboardShortcut}, specifying the shortcut string. */
 export interface KeyboardShortcutShortcutProps {
   readonly shortcut: string
+  readonly className?: string
 }
 
 /** Props for a {@link KeyboardShortcut}. */
@@ -112,10 +114,12 @@ export type KeyboardShortcutProps = KeyboardShortcutActionProps | KeyboardShortc
 
 /** A visual representation of a keyboard shortcut. */
 export default function KeyboardShortcut(props: KeyboardShortcutProps) {
+  const { className } = props
   const { getText } = useText()
   const inputBindings = useInputBindings()
   const shortcutString =
     'shortcut' in props ? props.shortcut : inputBindings.metadata[props.action].bindings[0]
+
   if (shortcutString == null) {
     return null
   } else {
@@ -125,7 +129,8 @@ export default function KeyboardShortcut(props: KeyboardShortcutProps) {
       <div
         className={twMerge(
           'flex items-center',
-          detect.isOnMacOS() ? 'gap-modifiers-macos' : 'gap-modifiers',
+          className,
+          detect.isOnMacOS() ? 'gap-[3px]' : 'gap-0.5',
         )}
       >
         {modifiers.map(
diff --git a/app/gui/src/dashboard/layouts/Settings/KeyboardShortcutsSettingsSection.tsx b/app/gui/src/dashboard/layouts/Settings/KeyboardShortcutsSettingsSection.tsx
index 76017e25aa9..d34a69d4ed2 100644
--- a/app/gui/src/dashboard/layouts/Settings/KeyboardShortcutsSettingsSection.tsx
+++ b/app/gui/src/dashboard/layouts/Settings/KeyboardShortcutsSettingsSection.tsx
@@ -45,7 +45,7 @@ export default function KeyboardShortcutsSettingsSection() {
 
   return (
     <>
-      <ButtonGroup>
+      <ButtonGroup className="grow-0">
         <DialogTrigger>
           <Button size="medium" variant="outline">
             {getText('resetAll')}
@@ -68,32 +68,25 @@ export default function KeyboardShortcutsSettingsSection() {
           <div
             {...mergeProps<JSX.IntrinsicElements['div']>()(innerProps, {
               ref: rootRef,
-              // There is a horizontal scrollbar for some reason without `px-px`.
-              className: 'overflow-auto px-px',
+              className: 'flex-1 min-h-0 overflow-auto',
               onScroll,
             })}
           >
             <table className="table-fixed border-collapse rounded-rows">
               <thead className="sticky top-0">
                 <tr className="h-row text-left text-sm font-semibold">
-                  <th className="pr-keyboard-shortcuts-icon-column-r min-w-keyboard-shortcuts-icon-column pl-cell-x">
-                    {/* Icon */}
-                  </th>
-                  <th className="min-w-keyboard-shortcuts-name-column px-cell-x">
-                    {getText('name')}
-                  </th>
+                  <th className="min-w-8 pl-cell-x pr-1.5">{/* Icon */}</th>
+                  <th className="min-w-36 px-cell-x">{getText('name')}</th>
                   <th className="px-cell-x">{getText('shortcuts')}</th>
-                  <th className="w-full min-w-keyboard-shortcuts-description-column px-cell-x">
-                    {getText('description')}
-                  </th>
+                  <th className="w-full min-w-64 px-cell-x">{getText('description')}</th>
                 </tr>
               </thead>
               <tbody ref={bodyRef}>
                 {visibleBindings.map((kv) => {
                   const [action, info] = kv
                   return (
-                    <tr key={action}>
-                      <td className="flex h-row items-center rounded-l-full bg-clip-padding pl-cell-x pr-icon-column-r">
+                    <tr key={action} className="rounded-rows-child">
+                      <td className="flex h-row items-center rounded-l-full bg-clip-padding pl-cell-x pr-1.5">
                         <SvgMask
                           src={info.icon ?? BlankIcon}
                           color={info.color}
@@ -112,11 +105,11 @@ export default function KeyboardShortcutsSettingsSection() {
                               {}
                               <div className="gap-buttons flex items-center pr-4">
                                 {info.bindings.map((binding, j) => (
-                                  <div
-                                    key={j}
-                                    className="inline-flex shrink-0 items-center gap-keyboard-shortcuts-button"
-                                  >
-                                    <KeyboardShortcut shortcut={binding} />
+                                  <div key={j} className="inline-flex shrink-0 items-center gap-1">
+                                    <KeyboardShortcut
+                                      shortcut={binding}
+                                      className="rounded-lg border-0.5 border-primary/10 px-1"
+                                    />
                                     <Button
                                       variant="ghost"
                                       size="medium"
@@ -132,7 +125,7 @@ export default function KeyboardShortcutsSettingsSection() {
                                   </div>
                                 ))}
                                 <div className="grow" />
-                                <div className="gap-keyboard-shortcuts-buttons flex shrink-0 items-center">
+                                <div className="flex shrink-0 items-center gap-1">
                                   <DialogTrigger>
                                     <Button
                                       variant="ghost"
diff --git a/app/gui/src/dashboard/layouts/Settings/MembersTable.tsx b/app/gui/src/dashboard/layouts/Settings/MembersTable.tsx
index 1cc89800367..170b689061d 100644
--- a/app/gui/src/dashboard/layouts/Settings/MembersTable.tsx
+++ b/app/gui/src/dashboard/layouts/Settings/MembersTable.tsx
@@ -140,7 +140,7 @@ export default function MembersTable(props: MembersTableProps) {
             {getText('email')}
           </Column>
           {/* Delete button. */}
-          {allowDelete && <Column className="w border-0" />}
+          {allowDelete && <Column className="w-0 border-0" />}
         </TableHeader>
         <TableBody ref={bodyRef} items={users ?? []} dependencies={[users]} className="select-text">
           {(member) => (
diff --git a/app/gui/src/dashboard/layouts/Settings/Section.tsx b/app/gui/src/dashboard/layouts/Settings/Section.tsx
index ffe1c88cacf..fb094cfac80 100644
--- a/app/gui/src/dashboard/layouts/Settings/Section.tsx
+++ b/app/gui/src/dashboard/layouts/Settings/Section.tsx
@@ -31,7 +31,7 @@ export default function SettingsSection(props: SettingsSectionProps) {
                 {getText(nameId)}
               </Text.Heading>
             )}
-            <div className="flex flex-col">
+            <div className="flex flex-col overflow-auto">
               {entries.map((entry, i) => (
                 <SettingsEntry key={i} context={context} data={entry} />
               ))}
diff --git a/app/gui/src/dashboard/layouts/Settings/Tab.tsx b/app/gui/src/dashboard/layouts/Settings/Tab.tsx
index 0c212f4403e..8ce53c6ab0b 100644
--- a/app/gui/src/dashboard/layouts/Settings/Tab.tsx
+++ b/app/gui/src/dashboard/layouts/Settings/Tab.tsx
@@ -64,7 +64,10 @@ export default function SettingsTab(props: SettingsTabProps) {
   } else {
     const content =
       columns.length === 1 ?
-        <div className="flex grow flex-col gap-settings-subsection" {...contentProps}>
+        <div
+          className={twMerge('flex grow flex-col gap-settings-subsection', classes[0])}
+          {...contentProps}
+        >
           {sections.map((section) => (
             <SettingsSection key={section.nameId} context={context} data={section} />
           ))}
diff --git a/app/gui/src/dashboard/layouts/Settings/data.tsx b/app/gui/src/dashboard/layouts/Settings/data.tsx
index 6277939740b..0645ebe4e44 100644
--- a/app/gui/src/dashboard/layouts/Settings/data.tsx
+++ b/app/gui/src/dashboard/layouts/Settings/data.tsx
@@ -385,6 +385,7 @@ export const SETTINGS_TAB_DATA: Readonly<Record<SettingsTabType, SettingsTabData
     sections: [
       {
         nameId: 'keyboardShortcutsSettingsSection',
+        columnClassName: 'h-full *:flex-1 *:min-h-0',
         entries: [
           {
             type: SettingsEntryType.custom,
diff --git a/app/gui/src/dashboard/tailwind.css b/app/gui/src/dashboard/tailwind.css
index 1acffaea9cb..1ad380d0813 100644
--- a/app/gui/src/dashboard/tailwind.css
+++ b/app/gui/src/dashboard/tailwind.css
@@ -97,8 +97,6 @@
   --spinner-fast-transition-duration: 1s;
   --spinner-medium-transition-duration: 5s;
   --spinner-slow-transition-duration: 90s;
-  --modifiers-gap: 0.1875rem;
-  --modifiers-macos-gap: 0.125rem;
   --cell-padding-x: 0.5rem;
   --plus-icon-size: 1.125rem;
   /* The horizontal gap between adjacent context menus. */
@@ -322,12 +320,6 @@
 
   --members-name-column-width: 12rem;
   --members-email-column-width: 12rem;
-  --keyboard-shortcuts-icon-column-width: 2rem;
-  --keyboard-shortcuts-name-column-width: 9rem;
-  --keyboard-shortcuts-description-column-width: 16rem;
-  --icon-column-padding-right: 0.375rem;
-  /* The horizontal gap between each icon for modifying the shortcuts for a particular action. */
-  --keyboard-shortcuts-button-gap: 0.25rem;
 
   /***********************\
   |* Authentication flow *|
diff --git a/app/gui/tailwind.config.js b/app/gui/tailwind.config.js
index 8872e4a8be3..787c88c1ce9 100644
--- a/app/gui/tailwind.config.js
+++ b/app/gui/tailwind.config.js
@@ -158,10 +158,6 @@ export default /** @satisfies {import('tailwindcss').Config} */ ({
 
         'members-name-column': 'var(--members-name-column-width)',
         'members-email-column': 'var(--members-email-column-width)',
-        'keyboard-shortcuts-icon-column': 'var(--keyboard-shortcuts-icon-column-width)',
-        'keyboard-shortcuts-name-column': 'var(--keyboard-shortcuts-name-column-width)',
-        'keyboard-shortcuts-description-column':
-          'var(--keyboard-shortcuts-description-column-width)',
         'drive-name-column': 'var(--drive-name-column-width)',
         'drive-modified-column': 'var(--drive-modified-column-width)',
         'drive-shared-with-column': 'var(--drive-shared-with-column-width)',
@@ -226,8 +222,6 @@ export default /** @satisfies {import('tailwindcss').Config} */ ({
         'settings-entry': 'var(--settings-entry-gap)',
         'settings-sidebar': 'var(--settings-sidebar-gap)',
         'new-empty-project': 'var(--new-empty-project-gap)',
-        modifiers: 'var(--modifiers-gap)',
-        'modifiers-macos': 'var(--modifiers-macos-gap)',
         'side-panel': 'var(--side-panel-gap)',
         'side-panel-section': 'var(--side-panel-section-gap)',
         'asset-search-bar': 'var(--asset-search-bar-gap)',
@@ -244,7 +238,6 @@ export default /** @satisfies {import('tailwindcss').Config} */ ({
         'context-menus': 'var(--context-menus-gap)',
         'asset-panel': 'var(--asset-panel-gap)',
         'search-suggestions': 'var(--search-suggestions-gap)',
-        'keyboard-shortcuts-button': 'var(--keyboard-shortcuts-button-gap)',
         'chat-buttons': 'var(--chat-buttons-gap)',
       },
       padding: {