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.
2024-09-11 20:37:54 +03:00 · 2024-07-31 12:48:46 +02:00 · 2024-07-31 12:48:46 +02:00 · daca1681c0
commit daca1681c0
parent 99724d0322
7 changed files with 101 additions and 60 deletions
--- 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>;
 };

--- 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}
+      />}
  </>;
 };

--- 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> {
--- 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>;
 };
--- 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>;
 };
--- 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);
--- 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'