Skip to content

Setting overflow: auto without a media query to fix resizable pane bug.#2685

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.

Sign up for GitHub

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

Merged
radglob merged 9 commits into from
Dec 19, 2022
Merged

Setting overflow: auto without a media query to fix resizable pane bug. #2685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a7b67c6
Setting overflow: auto without a media query to fix resizable pane bug.
radglob Dec 12, 2022
474fb87
Create cuddly-brooms-shake.md
radglob Dec 12, 2022
7771d2d
Update src/PageLayout/PageLayout.tsx
radglob Dec 12, 2022
90f55df
Updated snapshots.
radglob Dec 13, 2022
a251e0e
Add test to make sure we're not breaking resizable panes.
radglob Dec 13, 2022
d5045b1
Merge branch 'main' into bugfix/fix-splitpane-resizing
radglob Dec 13, 2022
9ae31a7
Update src/PageLayout/PageLayout.tsx
radglob Dec 19, 2022
c2d2b88
Merge branch 'main' into bugfix/fix-splitpane-resizing
radglob Dec 19, 2022
26d2501
Updated snapshots.
radglob Dec 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/cuddly-brooms-shake.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

Setting overflow: auto without a media query to fix resizable pane bug.
30 changes: 29 additions & 1 deletion src/PageLayout/PageLayout.test.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
import React from 'react'
import {act, render, screen} from '@testing-library/react'
import {act, fireEvent, render, screen} from '@testing-library/react'
import MatchMediaMock from 'jest-matchmedia-mock'
import 'react-intersection-observer/test-utils'
import {ThemeProvider} from '..'
import {viewportRanges} from '../hooks/useResponsiveValue'
import {PageLayout} from './PageLayout'
import {Placeholder} from '../Placeholder'

let matchMedia: MatchMediaMock

Expand DownExpand Up@@ -171,5 +172,32 @@ describe('PageLayout', () => {
)
expect(ref).toHaveBeenCalledWith(screen.getByTestId('content').parentNode)
})

it('should be resizable if `resizable` is set correctly', async () => {
render(
<ThemeProvider>
<PageLayout>
<PageLayout.Pane resizable>
<Placeholder height={320} label="Pane" />
</PageLayout.Pane>
<PageLayout.Content>
<Placeholder height={640} label="Content" />
</PageLayout.Content>
</PageLayout>
</ThemeProvider>,
)

const placeholder = await screen.findByText('Pane')
const pane = placeholder.parentNode
const initialWidth = (pane as HTMLElement).style.getPropertyValue('--pane-width')

const divider = await screen.findByRole('separator')
// Moving divider should resize pane.
fireEvent.mouseDown(divider)
fireEvent.mouseMove(divider)
fireEvent.mouseUp(divider)
Comment on lines +196 to +198

@radglobradglobDec 13, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a perfect test. I'm not sure how to configure this to move the divider a specific amount. However, as is, this does change the --pane-width value. fireEvent.click doesn't do that, so I think this is testing what we want.

const finalWidth = (pane as HTMLElement).style.getPropertyValue('--pane-width')
expect(finalWidth).not.toEqual(initialWidth)
})
})
})
5 changes: 2 additions & 3 deletions src/PageLayout/PageLayout.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -291,6 +291,7 @@ const VerticalDivider: React.FC<React.PropsWithChildren<DividerProps & Draggable
bg: isDragging ? 'accent.fg' : 'neutral.muted',
},
}}
role="separator"
onMouseDown={() => {
setIsDragging(true)
onDragStart?.()
Expand DownExpand Up@@ -675,9 +676,7 @@ const Pane = React.forwardRef<HTMLDivElement, React.PropsWithChildren<PageLayout
? ['100%', null, 'clamp(var(--pane-min-width), var(--pane-width), var(--pane-max-width))']
: paneWidths[width],
padding: SPACING_MAP[padding],
[`@media screen and (min-width: ${theme.breakpoints[1]})`]: {
overflow: 'auto',
},
overflow: [null, null, 'auto'],

[`@media screen and (min-width: ${theme.breakpoints[3]})`]: {
'--pane-max-width': 'calc(100vw - 959px)',
Expand Down
4 changes: 4 additions & 0 deletions src/PageLayout/__snapshots__/PageLayout.test.tsx.snap
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -132,6 +132,7 @@ exports[`PageLayout renders condensed layout 1`] = `

@media screen and (min-width:768px) {
.c11 {
width: 256px;
overflow: auto;
}
}
Expand DownExpand Up@@ -423,6 +424,7 @@ exports[`PageLayout renders default layout 1`] = `

@media screen and (min-width:768px) {
.c11 {
width: 256px;
overflow: auto;
}
}
Expand DownExpand Up@@ -715,6 +717,7 @@ exports[`PageLayout renders pane in different position when narrow 1`] = `

@media screen and (min-width:768px) {
.c11 {
width: 256px;
overflow: auto;
}
}
Expand DownExpand Up@@ -983,6 +986,7 @@ exports[`PageLayout renders with dividers 1`] = `

@media screen and (min-width:768px) {
.c10 {
width: 256px;
overflow: auto;
}
}
Expand Down
1 change: 1 addition & 0 deletions src/SplitPageLayout/__snapshots__/SplitPageLayout.test.tsx.snap
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -172,6 +172,7 @@ exports[`SplitPageLayout renders default layout 1`] = `

@media screen and (min-width:768px) {
.c11 {
width: 256px;
overflow: auto;
}
}
Expand Down