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

PageLayout: Fix layout shift for divider=line #4215

Closed
wants to merge 5 commits into from

Conversation

siddharthkp
Copy link
Member

@siddharthkp siddharthkp commented Feb 2, 2024

Problem

When using <PageLayout.Pane divider={{regular: 'line', narrow: 'none'}}>, we need to figure out which responsive value to use (regular, narrow or wide)

The hook useResponsiveValue we use renders the default (no divider) before the passed variant (like line) is rendered. This causes some layout shift (see video below)

search-shift.mov

This shift is slightly worse when the page is rendered on the server because this hook is not SSR compatible:

copilot-shift.mov

Note: This shift is also visible even when we have the same variant across screen sizes (<PageLayout.Pane divider="line">)

 

Changed

The ideal solution here would be to make this change css only, and not need any client side javascript. This however isn't very trivial and requires some time.

This PR has a pragmatic solution just for PageLayout to eliminate layout shift for the most common use case of <PageLayout.Pane divider="line"> by rendering a transparent divider for divider=none. This will hopefully buy us more time to fix the actual cause.

 

Before: There is no divider rendered at first, and then it renders and pushes the content away causing a layout shift

divider-before.mov

After: There is a transparent divider rendered at first, which does not cause any layout shift

divider-after.mov

 

Rollout strategy

  • Patch release

Testing & Reviewing

Merge checklist

  • Added/updated tests
    - [ ] Added/updated documentation
    - [ ] Added/updated previews (Storybook)
  • Changes are SSR compatible
  • Tested in Chrome
    - [ ] Tested in Firefox
    - [ ] Tested in Safari
    - [ ] Tested in Edge
  • (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

@siddharthkp siddharthkp requested review from a team and langermank February 2, 2024 21:42
Copy link

changeset-bot bot commented Feb 2, 2024

🦋 Changeset detected

Latest commit: c56be30

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 Patch

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

@siddharthkp siddharthkp requested a review from joshblack February 2, 2024 21:42
@siddharthkp siddharthkp self-assigned this Feb 2, 2024
Copy link
Contributor

github-actions bot commented Feb 2, 2024

size-limit report 📦

Path Size
dist/browser.esm.js 113.24 KB (+0.01% 🔺)
dist/browser.umd.js 113.89 KB (+0.01% 🔺)

Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Just wanted to confirm, are the changes to layout in the snapshots intended with this change or is the layout not expected to change in this way?

@siddharthkp siddharthkp marked this pull request as draft March 7, 2024 10:49
@siddharthkp
Copy link
Member Author

@joshblack Nah, those are all wrong 😢 Converted back to drafts

Copy link
Contributor

github-actions bot commented May 6, 2024

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 6, 2024
@siddharthkp
Copy link
Member Author

This was bigger than I initially anticipated, abandoning this

@siddharthkp siddharthkp closed this May 7, 2024
@siddharthkp siddharthkp deleted the pagelayout-stable-divider branch October 4, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch release bug fixes, docs, housekeeping Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VerticalDivider for PageLayout.Pane always initially renders as style 'none'
2 participants