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
18 changes: 13 additions & 5 deletions src/PageLayout/PageLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,9 @@ Content.displayName = 'PageLayout.Content'
// PageLayout.Pane

export type PageLayoutPaneProps = {
/**
* @deprecated position the pane by ordering your markup instead.
*/
position?: keyof typeof panePositions | ResponsiveValue<keyof typeof panePositions>
/**
* @deprecated Use the `position` prop with a responsive value instead.
Expand Down Expand Up @@ -563,11 +566,16 @@ 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.',
)
if (__DEV__ && process.env.NODE_ENV !== 'test') {
// We don't want these warnings to show up on tests because it fails the tests (at dotcom) due to not extecting a warning.
// Practically, this is not a conditional hook, it is just making sure this hook runs only on DEV not PROD.
// eslint-disable-next-line react-hooks/rules-of-hooks
React.useEffect(() => {
broccolinisoup marked this conversation as resolved.
Show resolved Hide resolved
warning(
responsivePosition !== undefined,
'The `position` prop will be removed on the next major version. You should order your markup as you want it to render instead.',
)
}, [responsivePosition])
}

// Combine position and positionWhenNarrow for backwards compatibility
Expand Down
10 changes: 6 additions & 4 deletions src/SplitPageLayout/SplitPageLayout.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ describe('SplitPageLayout', () => {
})

it('renders default layout', () => {
const spy = jest.spyOn(console, 'warn').mockImplementation(() => {})
// Disabling them for now because we are not expecting them in the tests.
// const spy = jest.spyOn(console, 'warn').mockImplementation(() => {})
broccolinisoup marked this conversation as resolved.
Show resolved Hide resolved
const {container} = render(
<ThemeProvider>
<SplitPageLayout>
Expand All @@ -28,21 +29,22 @@ describe('SplitPageLayout', () => {
</SplitPageLayout>
</ThemeProvider>,
)
expect(spy).toHaveBeenCalled()
// expect(spy).toHaveBeenCalled()

expect(container).toMatchSnapshot()
})

it('renders Pane with a custom ID', () => {
const spy = jest.spyOn(console, 'warn').mockImplementation(() => {})
// Disabling them for now because we are not expecting them in the tests.
// 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()
// expect(spy).toHaveBeenCalled()

const pane = getByText('Pane Content')

Expand Down