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.

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

Jump to bottom

fix(SelectPanel): Correctly recalculate position on overflow #5562

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

fix(SelectPanel): Correctly recalculate position on overflow #5562

wants to merge 32 commits into from
+274 −7

Conversation

francinelucca
Copy link
Member

@francinelucca francinelucca commented Jan 17, 2025 •
edited
Loading

Closes https://github.com/github/primer/issues/4027

Continuation of #5207.

When the selectPanel grows after opening (example: open in loading state, data is fetched, items are rendered), the position is not recalculated and may cause content to go off screen (See for more context https://github.com/github/primer/issues/4027). This PR corrects that undesired behavior by recalculating the position of the overlay on size change. Additionally, bake in logic so that the overlay retains it's size on filtering when anchored to the top (to prevent the panel from repositioning itself too many times).

before

Screen.Recording.2025-02-05.at.6.18.57.PM.mov
Screen.Recording.2025-02-05.at.6.19.47.PM.mov

after

Screen.Recording.2025-02-05.at.6.26.39.PM.mov
Screen.Recording.2025-02-05.at.6.21.45.PM.mov

Changelog

New

  • AnchoredOverlay dev stories for content resizing within page and dialog.
  • Support for pinPosition prop in AnchoredOverlay to prevent position shifting when sitting at the top of the anchor.
  • Support 'anchorSide' prop through 'overlayProps' in AnchoredOverlay
  • Adds max-height: 100vh default to AnchoredOverlay.
  • SelectPanel example stories for content resizing within page and dialog.
  • Support for width overlay prop in SelectPanel
  • Set pinPosition to true in SelectPanel if height hasn't been set by user
  • Added resizeObserver to useAnchoredPosition to watch for changes within the floating element (i.e., the overlay)

Changed

  • Add logic to useAnchoredPosition's updatePosition function to prevent anchored element to unpin from the top when possible (dependent on pinPosition) prop
  • Slight refactor in useResizeObserver hook
  • VRT updates (couldn't determine the source of these)

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Test new functionality in https://primer-88a2e13e51-13348165.drafts.github.io/storybook/?path=/story/components-selectpanel-examples--reposition-after-loading, https://primer-88a2e13e51-13348165.drafts.github.io/storybook/?path=/story/components-selectpanel-examples--select-panel-reposition-inside-dialog

Ensure default functionality still works as expected: https://primer-88a2e13e51-13348165.drafts.github.io/storybook/?path=/story/components-selectpanel--default

Merge checklist

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jan 17, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: c874e45

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

@github-actions github-actions bot added the staff Author is a staff member label Jan 17, 2025
@github-actions GitHub Actions
Copy link
Contributor

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Jan 17, 2025
@github-actions github-actions bot temporarily deployed to storybook-preview-5562 January 17, 2025 05:04 Inactive
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 17, 2025 •
edited
Loading

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 105.84 KB (+0.14% 🔺)
packages/react/dist/browser.umd.js 106.26 KB (+0.24% 🔺)

@github-actions github-actions bot temporarily deployed to storybook-preview-5562 January 23, 2025 00:05 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-5562 February 3, 2025 22:20 Inactive
francinelucca
francinelucca commented Feb 4, 2025
View reviewed changes
packages/react/src/hooks/useAnchoredPosition.ts
@@ -30,22 +31,49 @@ export function useAnchoredPosition(
const floatingElementRef = useProvidedRefOrCreate(settings?.floatingElementRef)
const anchorElementRef = useProvidedRefOrCreate(settings?.anchorElementRef)
const [position, setPosition] = React.useState<AnchorPosition | undefined>(undefined)
const [_, setPrevHeight] = React.useState<number | undefined>(undefined)
Copy link
Member Author

Choose a reason for hiding this comment

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

used to force the height of the selectPanel when filtered and it wants to shrink but it's anchored at the top (we want it to stay at the top and lock the height)

francinelucca
francinelucca commented Feb 4, 2025
View reviewed changes
packages/react/src/hooks/useAnchoredPosition.ts Outdated
Comment on lines 41 to 45
if (
settings?.pinPosition &&
prev &&
['outside-top', 'inside-top'].includes(prev.anchorSide) &&
(prev.anchorSide !== newPosition.anchorSide || prev.top < newPosition.top)
Copy link
Member Author

@francinelucca francinelucca Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

if the selectPanel is anchored to the top and it used to sit higher on the page....

francinelucca
francinelucca commented Feb 4, 2025
View reviewed changes
packages/react/src/hooks/useAnchoredPosition.ts Outdated
(prev.anchorSide !== newPosition.anchorSide || prev.top < newPosition.top)
) {
const anchorTop = anchorElementRef.current?.getBoundingClientRect().top ?? 0
if (anchorTop > (floatingElementRef.current?.clientHeight ?? 0)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

if the anchorTop is bigger than the selectPanel's height, that means there is plenty of space for the selectPanel to stay at the top

francinelucca
francinelucca commented Feb 4, 2025
View reviewed changes
packages/react/src/hooks/useAnchoredPosition.ts Outdated
if (anchorTop > (floatingElementRef.current?.clientHeight ?? 0)) {
setPrevHeight(prevHeight => {
if (prevHeight && prevHeight > (floatingElementRef.current?.clientHeight ?? 0)) {
requestAnimationFrame(() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

using requestAnimationFrame here to prevent a resizeObserver loop here due to us changing the element's height, but the observer listening for changes in the element...

francinelucca
francinelucca commented Feb 4, 2025
View reviewed changes
packages/react/src/hooks/useAnchoredPosition.ts Outdated
const anchorTop = anchorElementRef.current?.getBoundingClientRect().top ?? 0
if (anchorTop > (floatingElementRef.current?.clientHeight ?? 0)) {
setPrevHeight(prevHeight => {
if (prevHeight && prevHeight > (floatingElementRef.current?.clientHeight ?? 0)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

re-establish the height if it used to be bigger and is now attempting to shrink

francinelucca
francinelucca commented Feb 4, 2025
View reviewed changes
packages/react/src/hooks/useAnchoredPosition.ts Outdated
Comment on lines 54 to 55
} else {
prev = newPosition
Copy link
Member Author

Choose a reason for hiding this comment

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

edgecase where the selectPanel is actually growing instead of shrinking, in this case we do want to allow it to recalculate/reposition

@francinelucca francinelucca changed the title wip: SelectPanel overflow fix: SelectPanel overflow Feb 4, 2025
@francinelucca francinelucca changed the title fix: SelectPanel overflow fix(SelectPanel): Correctly recalculate position on overflow Feb 4, 2025
@github-actions github-actions bot removed the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Feb 13, 2025
@github-actions github-actions bot temporarily deployed to storybook-preview-5562 February 13, 2025 05:03 Inactive
@github-actions github-actions bot added integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm and removed update snapshots labels Feb 13, 2025
@github-actions GitHub Actions
Copy link
Contributor

👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks!

@github-actions github-actions bot added integration-tests: failing Changes in this PR cause breaking changes in gh/gh and removed integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh labels Feb 13, 2025
@primer primer bot temporarily deployed to github-pages February 13, 2025 05:23 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-5562 February 13, 2025 05:23 Inactive
@primer-integration
Copy link

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/362265

@github-actions github-actions bot temporarily deployed to storybook-preview-5562 February 14, 2025 00:58 Inactive
@primer-integration
Copy link

🟢 golden-jobs completed with status success.

@github-actions github-actions bot added integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh and removed integration-tests: failing Changes in this PR cause breaking changes in gh/gh labels Feb 14, 2025
@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Feb 14, 2025
@github-actions GitHub Actions
Copy link
Contributor

👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks!

@francinelucca
Copy link
Member Author

@camertron cleaned up the useAnchoredPosition logic and split into functions, lmk how that looks.
As for the flashing, I tried a couple things with the visibility like you suggested but couldn't get it to bypass it. I think visibility won't help in this case because the flashing happens when the SelectPanel is already in view, user filter so SelectPanel shrinks, and then we re-establish the height when the useAnchoredPosition hooks runs again as a response to the element resizing

@github-actions github-actions bot removed the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilybrick emilybrick emilybrick approved these changes

@tbenning tbenning Awaiting requested review from tbenning tbenning is a code owner automatically assigned from primer/design-reviewers

@camertron camertron Awaiting requested review from camertron

@TylerJDev TylerJDev Awaiting requested review from TylerJDev

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh staff Author is a staff member status: review needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@francinelucca @camertron @emilybrick @TylerJDev