Add resizable support to the `<PageLayout>` component #2352

japf · 2022-09-20T09:50:47Z

Describe your changes here.

This PR adds resizable support to the <PageLayout> component.

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

changeset-bot · 2022-09-20T09:50:49Z

🦋 Changeset detected

Latest commit: 6edaf11

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2022-09-20T09:53:55Z

size-limit report 📦

Path	Size
dist/browser.esm.js	78.8 KB (+0.85% 🔺)
dist/browser.umd.js	79.44 KB (+0.84% 🔺)

colebemis · 2022-09-20T16:30:01Z

This is looking great, @japf! 💖

I was poking around in the storybook and noticed that the pane width seems to "jump" when you start dragging. Any ideas why this might be happening?

CleanShot.2022-09-20.at.09.25.36.mp4

I see this PR is marked as "work in progress" so feel free to ignore if this is a known issue :)

japf · 2022-09-23T12:55:18Z

👋 @colebemis I pushed a few fixes, feel free to have another look. Please let me know what you think is missing to mark the PR as ready for review. I guess I should update the docs?

Edit: there is an issue with the positioning of the divider, I'll look into that.

japf · 2022-09-26T12:04:29Z

@colebemis updated again, please have another look 😄

src/PageLayout/PageLayout.tsx

colebemis · 2022-09-27T23:51:12Z

@japf this is looking great! @joshblack is going to take the lead on code review. He'll be your point-of-contact for any questions you have :)

joshblack · 2022-09-28T16:42:53Z

Looking great! 🥳 Thanks so much for taking this on 🙏 Just wanted to leave some notes/questions on behavior since I wasn't sure what was expected

SC seems to be generating class names per clamp value

It seems like Styled Components might be generating class names for each value in the clamp() 🤔 Would it be possible to pull the value out into an inline style or CSS Custom Property to avoid generating the class names? Or is there potentially something we can do with SC?

Screen.Recording.2022-09-28.at.11.27.33.AM.mov
What's the expectation for when the layout goes narrow?

I wasn't sure what the intent was for when the layout goes narrow, should the resize bar effectively be hidden at that breakpoint? Just wanted to double-check
Text selection when resizing on FF

Super minor, but noticed that text selection was happening for FF when hitting the edges of the resize behavior 🤔

Screen.Recording.2022-09-28.at.11.30.36.AM.mov
Safari on iPad

I wasn't sure how to get this to work when testing on iPad, it seems like the resize bar is interactive but when dragging it didn't seem to work (I guess because of onMouseDown?) Would we need to use something like onPointerDown to try and capture both cases or is the intent to drop down to touch events for iPad support?
Making the resize focusable

Was curious what direction we were thinking about for making this keyboard operable 👀
One idea I had was to give the resize bar role="separator" and tabindex="0" to make it focusable. We could then use the following attributes to communicate state + purpose as it changes:
- aria-label to label the resize bar
- aria-valuenow to set the number reflecting the current position
- aria-valuemin and aria-valuemax to represent the min/max values for resize
- aria-orientation="vertical" to reflect orientation
We could use onKeyDown on that element then and listen to ArrowLeft/ArrowRight to adjust it by px values and Shift+ArrowLeft, Shift+ArrowRight for increments of 10

Reference: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/separator_role#focusable_separator

Sorry for the long list here! Just wanted to say thanks again for taking this on and let me know if you have any questions or if I'm missing anything!

davekenn · 2022-10-03T15:10:09Z

Just a note that our new code view is getting feedback that our file tree is too narrow when searching for files (goto file). This resizing will solve that problem for us! Cant wait for this to get to production!

colebemis · 2022-10-19T17:43:07Z

Looks like this change causes the gap between the pane and content to be twice as large when there's no divider displayed:

colebemis · 2022-10-25T18:30:12Z

👋 @japf FYI I'm going to work on getting this over the finish line today. Planning to polish some of the interactions and styling.

japf · 2022-10-26T13:18:36Z

src/PageLayout/useHorizontalResize.tsx

+  useEffect(() => {
+    let hasValue = false
+    if (storageKey) {
+      const storedWidth = window.localStorage.getItem(storageKey)


As @mattcosta7 pointed out in https://github.com/github/issues/issues/3652 we might want to use a wrapped around the localStorage call to avoid throwing an error for folks who disabled 3rd party tracking in their browser.

Good call. Updated ✅

* Update stories * Refactor resizable pane implementation * Clamp pane width resizing * Delay drag hover effect * Reset pane width on double click * Handle styles while dragging * Add resizable example to SplitPageLayout docs * Remove comment * Add comment about touch events * Update docs * Update resizable pane story * Update snapshots

colebemis · 2022-10-27T01:45:07Z

Update: I finished addressing @joshblack's comments. Here's a quick video demo: https://share.cleanshot.com/imPRk8

This should be ready for another round of review. cc @joshblack

src/PageLayout/PageLayout.tsx

joshblack

Looks great! Noticed a couple odd behaviors when testing in Safari and Firefox, curious if you know what's going on @colebemis 🤔

Safari

Screen.Recording.2022-10-27.at.4.20.04.PM.mov

Firefox

Screen.Recording.2022-10-27.at.4.21.11.PM.mov

colebemis · 2022-10-27T23:31:23Z

@joshblack I think those odd behaviors are related to how we set the max width of the pane. The max width of the pane is relative to the entire viewport size (vw), which can lead to unexpected behavior when PageLayout is nested inside smaller containers (as seen in the docs). Do those odd behaviors occur in Firefox and Safari in the fullscreen storybook example?

colebemis · 2022-10-27T23:32:40Z

@joshblack I'm open to new ideas about how to handle min/max pane width if you have any! Here's the spec from @vdepizzol: https://github.com/github/primer/issues/581

…pf/resizable-pane

add resizable support to the PageLayout component

dacf38d

japf force-pushed the japf/resizable-pane branch from a687cc1 to dacf38d Compare September 20, 2022 09:54

Add changeset file

5e74d8b

japf temporarily deployed to github-pages September 20, 2022 10:05 Inactive

update snapshot

ffad694

japf temporarily deployed to github-pages September 20, 2022 12:32 Inactive

japf added 4 commits September 23, 2022 11:53

take client left position into account when pane is not left aligned

a5291c4

fix resize behavior when pane is not at start position

53ca22a

fix resize when there are column gaps

d12ba19

use story debug values

8ef210f

japf temporarily deployed to github-pages September 23, 2022 13:02 Inactive

decouple the resize handle from the vertical divider

9a7b498

japf force-pushed the japf/resizable-pane branch from f5d2fe9 to 9a7b498 Compare September 26, 2022 12:04

japf temporarily deployed to github-pages September 26, 2022 12:12 Inactive

japf added 2 commits September 26, 2022 14:21

update snap file

bbff181

update snap file

e808e0c

japf temporarily deployed to github-pages September 26, 2022 14:35 Inactive

colebemis reviewed Sep 27, 2022

View reviewed changes

src/PageLayout/PageLayout.tsx Outdated Show resolved Hide resolved

rename canResizePane to resizable

56413f8

japf temporarily deployed to github-pages September 28, 2022 11:30 Inactive

colebemis mentioned this pull request Oct 17, 2022

Dialog: Remove landmark elements #2444

Merged

Merge branch 'main' into japf/resizable-pane

64ff267

joshblack approved these changes Oct 19, 2022

View reviewed changes

japf commented Oct 26, 2022

View reviewed changes

colebemis temporarily deployed to github-pages October 27, 2022 01:47 Inactive

colebemis added 2 commits October 26, 2022 18:48

Wrap localStorage in try/catch

b282042

Merge branch 'main' into japf/resizable-pane

75cd2f3

colebemis temporarily deployed to github-pages October 27, 2022 01:57 Inactive

Merge branch 'main' into japf/resizable-pane

6a21e91

colebemis requested a review from joshblack October 27, 2022 15:06

colebemis temporarily deployed to github-pages October 27, 2022 15:12 Inactive

colebemis reviewed Oct 27, 2022

View reviewed changes

src/PageLayout/PageLayout.tsx Outdated Show resolved Hide resolved

Store pane width in px

78f0317

colebemis temporarily deployed to github-pages October 27, 2022 15:54 Inactive

joshblack reviewed Oct 27, 2022

View reviewed changes

joshblack approved these changes Oct 31, 2022

View reviewed changes

Merge branch 'main' into japf/resizable-pane

a9f81ac

colebemis temporarily deployed to github-pages October 31, 2022 22:31 Inactive

colebemis added 2 commits October 31, 2022 16:01

Update snapshot

d2076f5

Merge branch 'japf/resizable-pane' of github.com:primer/react into ja…

925fa0e

…pf/resizable-pane

colebemis temporarily deployed to github-pages October 31, 2022 23:09 Inactive

Update pagelayout snapshots

6edaf11

colebemis temporarily deployed to github-pages October 31, 2022 23:17 Inactive

colebemis merged commit 0c2db83 into main Oct 31, 2022

colebemis deleted the japf/resizable-pane branch October 31, 2022 23:17

primer-css mentioned this pull request Oct 31, 2022

Release tracking #2482

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resizable support to the `<PageLayout>` component #2352

Add resizable support to the `<PageLayout>` component #2352

japf commented Sep 20, 2022

changeset-bot bot commented Sep 20, 2022 •

edited

Loading

github-actions bot commented Sep 20, 2022 •

edited

Loading

colebemis commented Sep 20, 2022

japf commented Sep 23, 2022 •

edited

Loading

japf commented Sep 26, 2022

colebemis commented Sep 27, 2022

joshblack commented Sep 28, 2022 •

edited

Loading

davekenn commented Oct 3, 2022

colebemis commented Oct 19, 2022

colebemis commented Oct 25, 2022

japf Oct 26, 2022

colebemis Oct 27, 2022

colebemis commented Oct 27, 2022

joshblack left a comment •

edited

Loading

colebemis commented Oct 27, 2022

colebemis commented Oct 27, 2022

Add resizable support to the <PageLayout> component #2352

Add resizable support to the <PageLayout> component #2352

Conversation

japf commented Sep 20, 2022

Screenshots

Merge checklist

changeset-bot bot commented Sep 20, 2022 • edited Loading

🦋 Changeset detected

github-actions bot commented Sep 20, 2022 • edited Loading

size-limit report 📦

colebemis commented Sep 20, 2022

japf commented Sep 23, 2022 • edited Loading

japf commented Sep 26, 2022

colebemis commented Sep 27, 2022

joshblack commented Sep 28, 2022 • edited Loading

davekenn commented Oct 3, 2022

colebemis commented Oct 19, 2022

colebemis commented Oct 25, 2022

japf Oct 26, 2022

Choose a reason for hiding this comment

colebemis Oct 27, 2022

Choose a reason for hiding this comment

colebemis commented Oct 27, 2022

joshblack left a comment • edited Loading

Choose a reason for hiding this comment

colebemis commented Oct 27, 2022

colebemis commented Oct 27, 2022

Add resizable support to the `<PageLayout>` component #2352

Add resizable support to the `<PageLayout>` component #2352

changeset-bot bot commented Sep 20, 2022 •

edited

Loading

github-actions bot commented Sep 20, 2022 •

edited

Loading

japf commented Sep 23, 2022 •

edited

Loading

joshblack commented Sep 28, 2022 •

edited

Loading

joshblack left a comment •

edited

Loading