fix(PageLayout): Remove warning for the deprecated position prop (#…

…3545) * Add deprecated notice and run console warn only in DEV * run when the env is not test * Revert "Toggle switch a11y take3 (#3510)" This reverts commit bdbcfd1. * take the list changes back for underlinenav * lint and test fix * Add back "Toggle switch a11y take3 (#3510)" This reverts commit 5f94786. * Revert "take the list changes back for underlinenav" This reverts commit 843e304. * fix after revert commit * remove console warning and update docs and changeset * fix changeset and update prop docs
primer · Jul 27, 2023 · 430a203 · 430a203
1 parent 397938d
commit 430a203
Show file tree

Hide file tree

Showing 7 changed files with 47 additions and 26 deletions.
diff --git a/.changeset/hungry-spies-remember.md b/.changeset/hungry-spies-remember.md
@@ -2,6 +2,19 @@
 "@primer/react": patch
 ---
 
-Deprecates position prop for PageLayout.Pane. Users will receive a console warning when it is used.
+Deprecates `position` prop for PageLayout.Pane and SplitPageLayout.Pane.
 
-<!-- Changed components: PageLayout -->
+```diff
+-<PageLayout>
+-    <PageLayout.Content />
+-    <PageLayout.Pane position="start" />
+-</PageLayout>
+
++<PageLayout>
++    <PageLayout.Pane />
++    <PageLayout.Content />
++</PageLayout>
+
+```
+
+<!-- Changed components: PageLayout, SplitPageLayout -->
diff --git a/docs/content/PageLayout.mdx b/docs/content/PageLayout.mdx
@@ -87,12 +87,12 @@ See [storybook](https://primer.style/react/storybook?path=/story/components-page
   <PageLayout.Header>
     <Placeholder label="Header" height={64} />
   </PageLayout.Header>
+  <PageLayout.Pane>
+    <Placeholder label="Pane" height={120} />
+  </PageLayout.Pane>
   <PageLayout.Content>
     <Placeholder label="Content" height={240} />
   </PageLayout.Content>
-  <PageLayout.Pane position="start">
-    <Placeholder label="Pane" height={120} />
-  </PageLayout.Pane>
   <PageLayout.Footer>
     <Placeholder label="Footer" height={64} />
   </PageLayout.Footer>
@@ -106,12 +106,12 @@ See [storybook](https://primer.style/react/storybook?path=/story/components-page
   <PageLayout.Header>
     <Placeholder label="Header" height={64} />
   </PageLayout.Header>
+  <PageLayout.Pane hidden={{narrow: true}}>
+    <Placeholder label="Pane" height={120} />
+  </PageLayout.Pane>
   <PageLayout.Content>
     <Placeholder label="Content" height={240} />
   </PageLayout.Content>
-  <PageLayout.Pane position="start" hidden={{narrow: true}}>
-    <Placeholder label="Pane" height={120} />
-  </PageLayout.Pane>
   <PageLayout.Footer>
     <Placeholder label="Footer" height={64} />
   </PageLayout.Footer>
@@ -193,12 +193,12 @@ Add `offsetHeader` prop to specify the height of the custom sticky header along
     Custom sticky header
   </Box>
   <PageLayout>
+    <PageLayout.Pane sticky offsetHeader={64}>
+      <Placeholder label="Pane" height={120} />
+    </PageLayout.Pane>
     <PageLayout.Content>
       <Placeholder label="Content" height={320} />
     </PageLayout.Content>
-    <PageLayout.Pane position="start" sticky offsetHeader={64}>
-      <Placeholder label="Pane" height={120} />
-    </PageLayout.Pane>
     <PageLayout.Footer>
       <Placeholder label="Footer" height={64} />
     </PageLayout.Footer>
@@ -221,7 +221,7 @@ navigation container is used for.
   <PageLayout.Header>
     <Placeholder label="Header" height={64} />
   </PageLayout.Header>
-  <PageLayout.Pane position="start" aria-label="Secondary navigation">
+  <PageLayout.Pane aria-label="Secondary navigation">
     <NavList>
       <NavList.Item href="/" aria-current="page">
         Home

diff --git a/docs/content/SplitPageLayout.mdx b/docs/content/SplitPageLayout.mdx
@@ -167,7 +167,7 @@ If you need a more flexible layout component, consider using the [PageLayout](/P
     <SplitPageLayout.Content>
       <Placeholder label="Content" height={420} />
     </SplitPageLayout.Content>
-    <SplitPageLayout.Pane position="end">
+    <SplitPageLayout.Pane>
       <Placeholder label="Pane" height={120} />
     </SplitPageLayout.Pane>
     <SplitPageLayout.Footer>

diff --git a/src/PageLayout/PageLayout.docs.json b/src/PageLayout/PageLayout.docs.json
@@ -137,7 +137,7 @@
           "name": "position",
           "type": "| 'start' | 'end' | { narrow?: | 'start' | 'end' regular?: | 'start' | 'end' wide?: | 'start' | 'end' }",
           "defaultValue": "'end'",
-          "description": "",
+          "description": "Use source order instead of relying on the `position` prop",
           "deprecated": true
         },
         {

diff --git a/src/PageLayout/PageLayout.tsx b/src/PageLayout/PageLayout.tsx
@@ -482,6 +482,25 @@ Content.displayName = 'PageLayout.Content'
 // PageLayout.Pane
 
 export type PageLayoutPaneProps = {
+  /**
+   * @deprecated Use source order instead of relying on the `position` prop
+   *
+   * Before:
+   * ```
+   * <PageLayout>
+   *   <PageLayout.Content />
+   *   <PageLayout.Pane position="start" />
+   * </PageLayout>
+   * ```
+   *
+   * After:
+   * ```
+   * <PageLayout>
+   *   <PageLayout.Pane />
+   *   <PageLayout.Content />
+   * </PageLayout>
+   * ```
+   */
   position?: keyof typeof panePositions | ResponsiveValue<keyof typeof panePositions>
   /**
    * @deprecated Use the `position` prop with a responsive value instead.
@@ -563,13 +582,6 @@ const Pane = React.forwardRef<HTMLDivElement, React.PropsWithChildren<PageLayout
     },
     forwardRef,
   ) => {
-    if (responsivePosition !== undefined) {
-      // eslint-disable-next-line no-console
-      console.warn(
-        'The `position` prop will be removed on the next major version. You should order your markup as you want it to render instead.',
-      )
-    }
-
     // Combine position and positionWhenNarrow for backwards compatibility
     const positionProp =
       !isResponsiveValue(responsivePosition) && positionWhenNarrow !== 'inherit'

diff --git a/src/SplitPageLayout/SplitPageLayout.docs.json b/src/SplitPageLayout/SplitPageLayout.docs.json
@@ -72,7 +72,7 @@
           "name": "position",
           "type": "| 'start' | 'end' | { narrow?: | 'start' | 'end' regular?: | 'start' | 'end' wide?: | 'start' | 'end' }",
           "defaultValue": "start'",
-          "description": "",
+          "description": "Use source order instead of relying on the `position` prop",
           "deprecated": true
         },
         {

diff --git a/src/SplitPageLayout/SplitPageLayout.test.tsx b/src/SplitPageLayout/SplitPageLayout.test.tsx
@@ -17,7 +17,6 @@ describe('SplitPageLayout', () => {
   })
 
   it('renders default layout', () => {
-    const spy = jest.spyOn(console, 'warn').mockImplementation(() => {})
     const {container} = render(
       <ThemeProvider>
         <SplitPageLayout>
@@ -28,21 +27,18 @@ describe('SplitPageLayout', () => {
         </SplitPageLayout>
       </ThemeProvider>,
     )
-    expect(spy).toHaveBeenCalled()
 
     expect(container).toMatchSnapshot()
   })
 
   it('renders Pane with a custom ID', () => {
-    const spy = jest.spyOn(console, 'warn').mockImplementation(() => {})
     const {getByText} = render(
       <ThemeProvider>
         <SplitPageLayout>
           <SplitPageLayout.Pane id="customId">Pane Content</SplitPageLayout.Pane>
         </SplitPageLayout>
       </ThemeProvider>,
     )
-    expect(spy).toHaveBeenCalled()
 
     const pane = getByText('Pane Content')