Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[⏪ Reverted] fix(PageLayout): Remove warning for the deprecated position prop #3545

Merged
merged 11 commits into from
Jul 27, 2023
17 changes: 15 additions & 2 deletions .changeset/hungry-spies-remember.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 -->
20 changes: 10 additions & 10 deletions docs/content/PageLayout.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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>
Expand Down Expand Up @@ -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>
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion docs/content/SplitPageLayout.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
2 changes: 1 addition & 1 deletion src/PageLayout/PageLayout.docs.json
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
{
Expand Down
26 changes: 19 additions & 7 deletions src/PageLayout/PageLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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'
Expand Down
2 changes: 1 addition & 1 deletion src/SplitPageLayout/SplitPageLayout.docs.json
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
{
Expand Down
4 changes: 0 additions & 4 deletions src/SplitPageLayout/SplitPageLayout.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ describe('SplitPageLayout', () => {
})

it('renders default layout', () => {
const spy = jest.spyOn(console, 'warn').mockImplementation(() => {})
const {container} = render(
<ThemeProvider>
<SplitPageLayout>
Expand All @@ -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')

Expand Down