From 5731e3a15574cb1a063846bde059873a94f1a050 Mon Sep 17 00:00:00 2001
From: Michel Weststrate <mweststrate@fb.com>
Date: Fri, 23 Oct 2020 06:44:04 -0700
Subject: [PATCH] Scrolling improvements

Summary:
Split container had a convenient property `scrollable`, that automatically applies a Scroll container to the main content.

But I noticed it leads to bad design choices because it is so convenient. So sometimes scrolling would be unnecessarily in two directions because of this.

Or, since the scroll container wraps around the whole content, toolbars would scroll out of view. By forcing scrolling to be put explicitly in the component tree, we encourage plugin developers to think about where they actually want to have that scroll, and in which direction.

Also added options to use the Container padding properties on ScrollContainer, which is great since we can keep the scrollbars outside the padding, and apply it to the content only, to prevent an accidental mistake where people would put a scroll container in a padded container, that would put the scrollbar inside the padding.

Reviewed By: cekkaewnumchai

Differential Revision: D24502546

fbshipit-source-id: 524004a1c5f33a185f9b959251b72875dd623cb3
---
 desktop/app/src/chrome/ConsoleLogs.tsx        | 16 +++++-----
 .../src/sandy-chrome/DesignComponentDemos.tsx | 29 +++++++++++--------
 desktop/app/src/sandy-chrome/LeftSidebar.tsx  |  6 +++-
 .../sandy-chrome/appinspect/AppInspect.tsx    | 16 +++++-----
 desktop/app/src/ui/components/Layout.tsx      | 16 +++++-----
 desktop/plugins/sections/src/index.tsx        | 18 +++++++-----
 6 files changed, 56 insertions(+), 45 deletions(-)

diff --git a/desktop/app/src/chrome/ConsoleLogs.tsx b/desktop/app/src/chrome/ConsoleLogs.tsx
index 83239b180..427595f68 100644
--- a/desktop/app/src/chrome/ConsoleLogs.tsx
+++ b/desktop/app/src/chrome/ConsoleLogs.tsx
@@ -92,7 +92,7 @@ export function ConsoleLogs() {
   const styles = useMemo(() => buildTheme(isSandy), [isSandy]);
 
   return (
-    <Layout.Top scrollable>
+    <Layout.Top>
       <Toolbar>
         <ButtonGroup>
           <Button onClick={clearLogs} icon="trash">
@@ -101,12 +101,14 @@ export function ConsoleLogs() {
           <Button dropdown={dropdown}>Log Levels</Button>
         </ButtonGroup>
       </Toolbar>
-      <Console
-        logs={logs}
-        filter={logLevels}
-        variant={isDarkMode || !isSandy ? 'dark' : 'light'}
-        styles={styles}
-      />
+      <Layout.ScrollContainer vertical>
+        <Console
+          logs={logs}
+          filter={logLevels}
+          variant={isDarkMode || !isSandy ? 'dark' : 'light'}
+          styles={styles}
+        />
+      </Layout.ScrollContainer>
     </Layout.Top>
   );
 }
diff --git a/desktop/app/src/sandy-chrome/DesignComponentDemos.tsx b/desktop/app/src/sandy-chrome/DesignComponentDemos.tsx
index 073eaee6a..6f69e3c1c 100644
--- a/desktop/app/src/sandy-chrome/DesignComponentDemos.tsx
+++ b/desktop/app/src/sandy-chrome/DesignComponentDemos.tsx
@@ -179,6 +179,11 @@ const demos: PreviewProps[] = [
         'boolean',
         'specifies in which directions the container should scroll. If none is specified the container will scroll in both directions',
       ],
+      [
+        'padv / padh / pad',
+        'see Container',
+        'Padding will be applied to the child',
+      ],
     ],
     demos: {
       'Basic usage': (
@@ -246,34 +251,34 @@ const demos: PreviewProps[] = [
           {aFixedHeightBox}
         </Layout.Bottom>
       ),
-      'Layout.Top + scrollable': (
+      'Layout.Top + Layout.ScrollContainer': (
         <Layout.Container style={{height: 150}}>
-          <Layout.Top scrollable>
+          <Layout.Top>
             {aFixedHeightBox}
-            {largeChild}
+            <Layout.ScrollContainer>{largeChild}</Layout.ScrollContainer>
           </Layout.Top>
         </Layout.Container>
       ),
-      'Layout.Left + scrollable': (
+      'Layout.Left + Layout.ScrollContainer': (
         <Layout.Container style={{height: 150}}>
-          <Layout.Left scrollable>
+          <Layout.Left>
             {aFixedWidthBox}
-            {largeChild}
+            <Layout.ScrollContainer>{largeChild}</Layout.ScrollContainer>
           </Layout.Left>
         </Layout.Container>
       ),
-      'Layout.Right + scrollable': (
+      'Layout.Right + Layout.ScrollContainer': (
         <Layout.Container style={{height: 150}}>
-          <Layout.Right scrollable>
-            {largeChild}
+          <Layout.Right>
+            <Layout.ScrollContainer>{largeChild}</Layout.ScrollContainer>
             {aFixedWidthBox}
           </Layout.Right>
         </Layout.Container>
       ),
-      'Layout.Bottom + scrollable': (
+      'Layout.Bottom + Layout.ScrollContainer': (
         <Layout.Container style={{height: 150}}>
-          <Layout.Bottom scrollable>
-            {largeChild}
+          <Layout.Bottom>
+            <Layout.ScrollContainer>{largeChild}</Layout.ScrollContainer>
             {aFixedHeightBox}
           </Layout.Bottom>
         </Layout.Container>
diff --git a/desktop/app/src/sandy-chrome/LeftSidebar.tsx b/desktop/app/src/sandy-chrome/LeftSidebar.tsx
index 86b47d5ed..9f0187684 100644
--- a/desktop/app/src/sandy-chrome/LeftSidebar.tsx
+++ b/desktop/app/src/sandy-chrome/LeftSidebar.tsx
@@ -15,7 +15,11 @@ import {Button, Tooltip, Typography} from 'antd';
 import {InfoCircleOutlined} from '@ant-design/icons';
 
 export const LeftSidebar: React.FC = ({children}) => (
-  <Layout.Container borderRight padv="small" grow shrink>
+  <Layout.Container
+    borderRight
+    style={{paddingTop: theme.space.small}}
+    grow
+    shrink>
     {children}
   </Layout.Container>
 );
diff --git a/desktop/app/src/sandy-chrome/appinspect/AppInspect.tsx b/desktop/app/src/sandy-chrome/appinspect/AppInspect.tsx
index 920603204..c6369e0df 100644
--- a/desktop/app/src/sandy-chrome/appinspect/AppInspect.tsx
+++ b/desktop/app/src/sandy-chrome/appinspect/AppInspect.tsx
@@ -57,15 +57,13 @@ export function AppInspect() {
             </Layout.Horizontal>
           </Layout.Container>
         </Layout.Container>
-        <Layout.Container padv={theme.space.large}>
-          <Layout.ScrollContainer vertical>
-            {selectedDevice ? (
-              <PluginList />
-            ) : (
-              <Alert message="No device or app selected" type="info" />
-            )}
-          </Layout.ScrollContainer>
-        </Layout.Container>
+        <Layout.ScrollContainer vertical padv={theme.space.large}>
+          {selectedDevice ? (
+            <PluginList />
+          ) : (
+            <Alert message="No device or app selected" type="info" />
+          )}
+        </Layout.ScrollContainer>
       </Layout.Top>
     </LeftSidebar>
   );
diff --git a/desktop/app/src/ui/components/Layout.tsx b/desktop/app/src/ui/components/Layout.tsx
index a8b53b265..e2a53757e 100644
--- a/desktop/app/src/ui/components/Layout.tsx
+++ b/desktop/app/src/ui/components/Layout.tsx
@@ -116,25 +116,26 @@ const ScrollContainer = ({
   children,
   horizontal,
   vertical,
+  padv,
+  padh,
+  pad,
   ...rest
 }: React.HTMLAttributes<HTMLDivElement> & {
   horizontal?: boolean;
   vertical?: boolean;
-}) => {
+} & PaddingProps) => {
   const axis =
     horizontal && !vertical ? 'x' : !horizontal && vertical ? 'y' : 'both';
   return (
     <ScrollParent axis={axis} {...rest}>
-      <ScrollChild axis={axis}>{children}</ScrollChild>
+      <ScrollChild axis={axis} padv={padv} padh={padh} pad={pad}>
+        {children}
+      </ScrollChild>
     </ScrollParent>
   ) as any;
 };
 
 type SplitLayoutProps = {
-  /**
-   * If set, the dynamically sized pane will get scrollbars when needed
-   */
-  scrollable?: boolean;
   /**
    * If set, items will be centered over the orthogonal direction, if false (the default) items will be stretched.
    */
@@ -150,8 +151,7 @@ function renderSplitLayout(
   // eslint-disable-next-line
   const isSandy = useIsSandy();
   if (!isSandy) return renderLayout(props, direction === 'row', grow === 1);
-  let [child1, child2] = props.children;
-  if (props.scrollable) child2 = <ScrollContainer>{child2}</ScrollContainer>;
+  const [child1, child2] = props.children;
   return (
     <SandySplitContainer {...props} flexDirection={direction} grow={grow}>
       {child1}
diff --git a/desktop/plugins/sections/src/index.tsx b/desktop/plugins/sections/src/index.tsx
index 00318c15e..95b61628a 100644
--- a/desktop/plugins/sections/src/index.tsx
+++ b/desktop/plugins/sections/src/index.tsx
@@ -256,7 +256,7 @@ export function Component() {
             </Button>
           )}
         </Toolbar>
-        <Layout.Top scrollable={false}>
+        <Layout.Top>
           <Sidebar position="top" minHeight={80} height={80}>
             <EventTable
               generations={filteredGenerations}
@@ -270,7 +270,7 @@ export function Component() {
               }}
             />
           </Sidebar>
-          <Layout.Top scrollable={true}>
+          <Layout.Top>
             <Sidebar position="top" minHeight={400} height={400}>
               <TreeContainer>
                 <TreeHierarchy
@@ -282,12 +282,14 @@ export function Component() {
               </TreeContainer>
             </Sidebar>
             {focusedTreeGeneration && (
-              <StackTrace
-                data={focusedTreeGeneration.stack_trace}
-                skipStackTraceFormat={
-                  focusedTreeGeneration.skip_stack_trace_format
-                }
-              />
+              <Layout.ScrollContainer>
+                <StackTrace
+                  data={focusedTreeGeneration.stack_trace}
+                  skipStackTraceFormat={
+                    focusedTreeGeneration.skip_stack_trace_format
+                  }
+                />
+              </Layout.ScrollContainer>
             )}
           </Layout.Top>
         </Layout.Top>