From daca1681c0cb83de82a3c3263f2cf24226f0873a Mon Sep 17 00:00:00 2001
From: Simon Knott <info@simonknott.de>
Date: Wed, 31 Jul 2024 12:48:46 +0200
Subject: [PATCH] refactor(ui): in splitview component, move sidebar and main
 from children into named properties (#31925)

Pulled out from https://github.com/microsoft/playwright/pull/31900

I stumbled over `React.Children`, because it's the first time I saw that
used. https://react.dev/reference/react/Children lists `React.Children`
it as "Legacy" and mentions it's uncommon. Also, the fact that SplitView
only displays its first two children, and all others are silently
discarded, can be a surprise to some.

By separating things out into `sidebar` and `main`, not only do we give
the two elements names (otherwise one needs to remember that sidebar is
always the first child), but we also prevent any "third children" from
being dropped.
---
 packages/recorder/src/recorder.tsx            | 15 ++---
 packages/trace-viewer/src/ui/networkTab.tsx   | 13 +++--
 packages/trace-viewer/src/ui/sourceTab.tsx    | 13 +++--
 packages/trace-viewer/src/ui/uiModeView.tsx   | 16 ++++--
 packages/trace-viewer/src/ui/workbench.tsx    | 34 +++++++-----
 .../web/src/components/splitView.spec.tsx     | 55 ++++++++++++-------
 packages/web/src/components/splitView.tsx     | 15 +++--
 7 files changed, 101 insertions(+), 60 deletions(-)

diff --git a/packages/recorder/src/recorder.tsx b/packages/recorder/src/recorder.tsx
index 3b47c7d085..b3966fd01a 100644
--- a/packages/recorder/src/recorder.tsx
+++ b/packages/recorder/src/recorder.tsx
@@ -167,26 +167,27 @@ export const Recorder: React.FC<RecorderProps> = ({
       }}></ToolbarButton>
       <ToolbarButton icon='color-mode' title='Toggle color mode' toggled={false} onClick={() => toggleTheme()}></ToolbarButton>
     </Toolbar>
-    <SplitView sidebarSize={200}>
-      <CodeMirrorWrapper text={source.text} language={source.language} highlight={source.highlight} revealLine={source.revealLine} readOnly={true} lineNumbers={true}/>
-      <TabbedPane
+    <SplitView
+      sidebarSize={200}
+      main={<CodeMirrorWrapper text={source.text} language={source.language} highlight={source.highlight} revealLine={source.revealLine} readOnly={true} lineNumbers={true} />}
+      sidebar={<TabbedPane
         rightToolbar={selectedTab === 'locator' ? [<ToolbarButton icon='files' title='Copy' onClick={() => copy(locator)} />] : []}
         tabs={[
           {
             id: 'locator',
             title: 'Locator',
-            render: () => <CodeMirrorWrapper text={locator} language={source.language} readOnly={false} focusOnChange={true} onChange={onEditorChange} wrapLines={true}/>
+            render: () => <CodeMirrorWrapper text={locator} language={source.language} readOnly={false} focusOnChange={true} onChange={onEditorChange} wrapLines={true} />
           },
           {
             id: 'log',
             title: 'Log',
-            render: () => <CallLogView language={source.language} log={Array.from(log.values())}/>
+            render: () => <CallLogView language={source.language} log={Array.from(log.values())} />
           },
         ]}
         selectedTab={selectedTab}
         setSelectedTab={setSelectedTab}
-      />
-    </SplitView>
+      />}
+    />
   </div>;
 };
 
diff --git a/packages/trace-viewer/src/ui/networkTab.tsx b/packages/trace-viewer/src/ui/networkTab.tsx
index 311af733a9..3158ad0e67 100644
--- a/packages/trace-viewer/src/ui/networkTab.tsx
+++ b/packages/trace-viewer/src/ui/networkTab.tsx
@@ -101,10 +101,15 @@ export const NetworkTab: React.FunctionComponent<{
   />;
   return <>
     {!selectedEntry && grid}
-    {selectedEntry && <SplitView sidebarSize={columnWidths.get('name')!} sidebarIsFirst={true} orientation='horizontal' settingName='networkResourceDetails'>
-      <NetworkResourceDetails resource={selectedEntry.resource} onClose={() => setSelectedEntry(undefined)} />
-      {grid}
-    </SplitView>}
+    {selectedEntry &&
+      <SplitView
+        sidebarSize={columnWidths.get('name')!}
+        sidebarIsFirst={true}
+        orientation='horizontal'
+        settingName='networkResourceDetails'
+        main={<NetworkResourceDetails resource={selectedEntry.resource} onClose={() => setSelectedEntry(undefined)} />}
+        sidebar={grid}
+      />}
   </>;
 };
 
diff --git a/packages/trace-viewer/src/ui/sourceTab.tsx b/packages/trace-viewer/src/ui/sourceTab.tsx
index 27d64146ec..121b226216 100644
--- a/packages/trace-viewer/src/ui/sourceTab.tsx
+++ b/packages/trace-viewer/src/ui/sourceTab.tsx
@@ -96,17 +96,20 @@ export const SourceTab: React.FunctionComponent<{
 
   const showStackFrames = (stack?.length ?? 0) > 1;
 
-  return <SplitView sidebarSize={200} orientation={stackFrameLocation === 'bottom' ? 'vertical' : 'horizontal'} sidebarHidden={!showStackFrames}>
-    <div className='vbox' data-testid='source-code'>
+  return <SplitView
+    sidebarSize={200}
+    orientation={stackFrameLocation === 'bottom' ? 'vertical' : 'horizontal'}
+    sidebarHidden={!showStackFrames}
+    main={<div className='vbox' data-testid='source-code'>
       { fileName && <Toolbar>
         <span className='source-tab-file-name'>{fileName}</span>
         <CopyToClipboard description='Copy filename' value={getFileName(fileName, targetLine)}/>
         {location && <ToolbarButton icon='link-external' title='Open in VS Code' onClick={openExternally}></ToolbarButton>}
       </Toolbar> }
       <CodeMirrorWrapper text={source.content || ''} language='javascript' highlight={highlight} revealLine={targetLine} readOnly={true} lineNumbers={true} />
-    </div>
-    <StackTraceView stack={stack} selectedFrame={selectedFrame} setSelectedFrame={setSelectedFrame} />
-  </SplitView>;
+    </div>}
+    sidebar={<StackTraceView stack={stack} selectedFrame={selectedFrame} setSelectedFrame={setSelectedFrame} />}
+  />;
 };
 
 export async function calculateSha1(text: string): Promise<string> {
diff --git a/packages/trace-viewer/src/ui/uiModeView.tsx b/packages/trace-viewer/src/ui/uiModeView.tsx
index 98d19649eb..3484f3c749 100644
--- a/packages/trace-viewer/src/ui/uiModeView.tsx
+++ b/packages/trace-viewer/src/ui/uiModeView.tsx
@@ -433,8 +433,13 @@ export const UIModeView: React.FC<{}> = ({
       <div className='title'>UI Mode disconnected</div>
       <div><a href='#' onClick={() => window.location.href = '/'}>Reload the page</a> to reconnect</div>
     </div>}
-    <SplitView sidebarSize={250} minSidebarSize={150} orientation='horizontal' sidebarIsFirst={true} settingName='testListSidebar'>
-      <div className='vbox'>
+    <SplitView
+      sidebarSize={250}
+      minSidebarSize={150}
+      orientation='horizontal'
+      sidebarIsFirst={true}
+      settingName='testListSidebar'
+      main={<div className='vbox'>
         <div className={clsx('vbox', !isShowingOutput && 'hidden')}>
           <Toolbar>
             <div className='section-title' style={{ flex: 'none' }}>Output</div>
@@ -452,8 +457,8 @@ export const UIModeView: React.FC<{}> = ({
             onOpenExternally={location => testServerConnection?.openNoReply({ location: { file: location.file, line: location.line, column: location.column } })}
           />
         </div>
-      </div>
-      <div className='vbox ui-mode-sidebar'>
+      </div>}
+      sidebar={<div className='vbox ui-mode-sidebar'>
         <Toolbar noShadow={true} noMinHeight={true}>
           <img src='playwright-logo.svg' alt='Playwright logo' />
           <div className='section-title'>Playwright</div>
@@ -530,6 +535,7 @@ export const UIModeView: React.FC<{}> = ({
           showRouteActionsSetting,
         ]} />}
       </div>
-    </SplitView>
+      }
+    />
   </div>;
 };
diff --git a/packages/trace-viewer/src/ui/workbench.tsx b/packages/trace-viewer/src/ui/workbench.tsx
index 189e9265a9..a213d15c59 100644
--- a/packages/trace-viewer/src/ui/workbench.tsx
+++ b/packages/trace-viewer/src/ui/workbench.tsx
@@ -293,9 +293,15 @@ export const Workbench: React.FunctionComponent<{
       selectedTime={selectedTime}
       setSelectedTime={setSelectedTime}
     />
-    <SplitView sidebarSize={250} orientation={sidebarLocation === 'bottom' ? 'vertical' : 'horizontal'} settingName='propertiesSidebar'>
-      <SplitView sidebarSize={250} orientation='horizontal' sidebarIsFirst={true} settingName='actionListSidebar'>
-        <SnapshotTab
+    <SplitView
+      sidebarSize={250}
+      orientation={sidebarLocation === 'bottom' ? 'vertical' : 'horizontal'} settingName='propertiesSidebar'
+      main={<SplitView
+        sidebarSize={250}
+        orientation='horizontal'
+        sidebarIsFirst
+        settingName='actionListSidebar'
+        main={<SnapshotTab
           action={activeAction}
           sdkLanguage={sdkLanguage}
           testIdAttributeName={model?.testIdAttributeName || 'data-testid'}
@@ -303,14 +309,16 @@ export const Workbench: React.FunctionComponent<{
           setIsInspecting={setIsInspecting}
           highlightedLocator={highlightedLocator}
           setHighlightedLocator={locatorPicked}
-          openPage={openPage} />
-        <TabbedPane
-          tabs={showSettings ? [actionsTab, metadataTab, settingsTab] : [actionsTab, metadataTab]}
-          selectedTab={selectedNavigatorTab}
-          setSelectedTab={setSelectedNavigatorTab}
-        />
-      </SplitView>
-      <TabbedPane
+          openPage={openPage} />}
+        sidebar={
+          <TabbedPane
+            tabs={showSettings ? [actionsTab, metadataTab, settingsTab] : [actionsTab, metadataTab]}
+            selectedTab={selectedNavigatorTab}
+            setSelectedTab={setSelectedNavigatorTab}
+          />
+        }
+      />}
+      sidebar={<TabbedPane
         tabs={tabs}
         selectedTab={selectedPropertiesTab}
         setSelectedTab={selectPropertiesTab}
@@ -324,7 +332,7 @@ export const Workbench: React.FunctionComponent<{
             }} />
         ]}
         mode={sidebarLocation === 'bottom' ? 'default' : 'select'}
-      />
-    </SplitView>
+      />}
+    />
   </div>;
 };
diff --git a/packages/web/src/components/splitView.spec.tsx b/packages/web/src/components/splitView.spec.tsx
index fc376bd2ef..a9260a2d48 100644
--- a/packages/web/src/components/splitView.spec.tsx
+++ b/packages/web/src/components/splitView.spec.tsx
@@ -20,10 +20,12 @@ import { SplitView } from './splitView';
 test.use({ viewport: { width: 500, height: 500 } });
 
 test('should render', async ({ mount }) => {
-  const component = await mount(<SplitView sidebarSize={100}>
-    <div id='main' style={{ border: '1px solid red', flex: 'auto' }}>main</div>
-    <div id='sidebar' style={{ border: '1px solid blue', flex: 'auto' }}>sidebar</div>
-  </SplitView>);
+  const component = await mount(
+      <SplitView
+        sidebarSize={100}
+        main={<div id='main' style={{ border: '1px solid red', flex: 'auto' }}>main</div>}
+        sidebar={<div id='sidebar' style={{ border: '1px solid blue', flex: 'auto' }}>sidebar</div>}
+      />);
   const mainBox = await component.locator('#main').boundingBox();
   const sidebarBox = await component.locator('#sidebar').boundingBox();
   expect.soft(mainBox).toEqual({ x: 0, y: 0, width: 500, height: 400 });
@@ -31,10 +33,13 @@ test('should render', async ({ mount }) => {
 });
 
 test('should render sidebar first', async ({ mount }) => {
-  const component = await mount(<SplitView sidebarSize={100} sidebarIsFirst={true}>
-    <div id='main' style={{ border: '1px solid blue', flex: 'auto' }}>main</div>
-    <div id='sidebar' style={{ border: '1px solid red', flex: 'auto' }}>sidebar</div>
-  </SplitView>);
+  const component = await mount(
+      <SplitView
+        sidebarSize={100}
+        sidebarIsFirst
+        main={<div id='main' style={{ border: '1px solid blue', flex: 'auto' }}>main</div>}
+        sidebar={<div id='sidebar' style={{ border: '1px solid red', flex: 'auto' }}>sidebar</div>}
+      />);
   const mainBox = await component.locator('#main').boundingBox();
   const sidebarBox = await component.locator('#sidebar').boundingBox();
   expect.soft(mainBox).toEqual({ x: 0, y: 100, width: 500, height: 400 });
@@ -42,10 +47,14 @@ test('should render sidebar first', async ({ mount }) => {
 });
 
 test('should render horizontal split', async ({ mount }) => {
-  const component = await mount(<SplitView sidebarSize={100} sidebarIsFirst={true} orientation={'horizontal'}>
-    <div id='main' style={{ border: '1px solid blue', flex: 'auto' }}>main</div>
-    <div id='sidebar' style={{ border: '1px solid red', flex: 'auto' }}>sidebar</div>
-  </SplitView>);
+  const component = await mount(
+      <SplitView
+        sidebarSize={100}
+        sidebarIsFirst
+        orientation='horizontal'
+        main={<div id='main' style={{ border: '1px solid blue', flex: 'auto' }}>main</div>}
+        sidebar={<div id='sidebar' style={{ border: '1px solid red', flex: 'auto' }}>sidebar</div>}
+      />);
   const mainBox = await component.locator('#main').boundingBox();
   const sidebarBox = await component.locator('#sidebar').boundingBox();
   expect.soft(mainBox).toEqual({ x: 100, y: 0, width: 400, height: 500 });
@@ -53,19 +62,25 @@ test('should render horizontal split', async ({ mount }) => {
 });
 
 test('should hide sidebar', async ({ mount }) => {
-  const component = await mount(<SplitView sidebarSize={100} orientation={'horizontal'} sidebarHidden={true}>
-    <div id='main' style={{ border: '1px solid blue', flex: 'auto' }}>main</div>
-    <div id='sidebar' style={{ border: '1px solid red', flex: 'auto' }}>sidebar</div>
-  </SplitView>);
+  const component = await mount(
+      <SplitView
+        sidebarSize={100}
+        orientation={'horizontal'}
+        sidebarHidden
+        main={<div id='main' style={{ border: '1px solid blue', flex: 'auto' }}>main</div>}
+        sidebar={<div id='sidebar' style={{ border: '1px solid red', flex: 'auto' }}>sidebar</div>}
+      />);
   const mainBox = await component.locator('#main').boundingBox();
   expect.soft(mainBox).toEqual({ x: 0, y: 0, width: 500, height: 500 });
 });
 
 test('drag resize', async ({ page, mount }) => {
-  const component = await mount(<SplitView sidebarSize={100}>
-    <div id='main' style={{ border: '1px solid blue', flex: 'auto' }}>main</div>
-    <div id='sidebar' style={{ border: '1px solid red', flex: 'auto' }}>sidebar</div>
-  </SplitView>);
+  const component = await mount(
+      <SplitView
+        sidebarSize={100}
+        main={<div id='main' style={{ border: '1px solid blue', flex: 'auto' }}>main</div>}
+        sidebar={<div id='sidebar' style={{ border: '1px solid red', flex: 'auto' }}>sidebar</div>}
+      />);
   await page.mouse.move(25, 400);
   await page.mouse.down();
   await page.mouse.move(25, 100);
diff --git a/packages/web/src/components/splitView.tsx b/packages/web/src/components/splitView.tsx
index 4c4513f235..776b8d842f 100644
--- a/packages/web/src/components/splitView.tsx
+++ b/packages/web/src/components/splitView.tsx
@@ -25,18 +25,22 @@ export type SplitViewProps = {
   orientation?: 'vertical' | 'horizontal';
   minSidebarSize?: number;
   settingName?: string;
+
+  sidebar: React.ReactNode;
+  main: React.ReactNode;
 };
 
 const kMinSize = 50;
 
-export const SplitView: React.FC<React.PropsWithChildren<SplitViewProps>> = ({
+export const SplitView: React.FC<SplitViewProps> = ({
   sidebarSize,
   sidebarHidden = false,
   sidebarIsFirst = false,
   orientation = 'vertical',
   minSidebarSize = kMinSize,
   settingName,
-  children
+  sidebar,
+  main,
 }) => {
   const defaultSize = Math.max(minSidebarSize, sidebarSize) * window.devicePixelRatio;
   const hSetting = useSetting<number>((settingName ?? 'unused') + '.' + orientation + ':size', defaultSize);
@@ -60,7 +64,6 @@ export const SplitView: React.FC<React.PropsWithChildren<SplitViewProps>> = ({
       size = measure.width - 10;
   }
 
-  const childrenArray = React.Children.toArray(children);
   document.body.style.userSelect = resizing ? 'none' : 'inherit';
   let resizerStyle: any = {};
   if (orientation === 'vertical') {
@@ -75,9 +78,9 @@ export const SplitView: React.FC<React.PropsWithChildren<SplitViewProps>> = ({
       resizerStyle = { right: resizing ? 0 : size - 4, left: resizing ? 0 : undefined, width: resizing ? 'initial' : 8 };
   }
 
-  return <div className={clsx('split-view', orientation, sidebarIsFirst && 'sidebar-first') } ref={ref}>
-    <div className='split-view-main'>{childrenArray[0]}</div>
-    { !sidebarHidden && <div style={{ flexBasis: size }} className='split-view-sidebar'>{childrenArray[1]}</div> }
+  return <div className={clsx('split-view', orientation, sidebarIsFirst && 'sidebar-first')} ref={ref}>
+    <div className='split-view-main'>{main}</div>
+    { !sidebarHidden && <div style={{ flexBasis: size }} className='split-view-sidebar'>{sidebar}</div> }
     { !sidebarHidden && <div
       style={resizerStyle}
       className='split-view-resizer'