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

Scroll AnchoredOverlay when it's inside a scrolling child of the body #4195

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

owenniblock
Copy link
Contributor

@owenniblock owenniblock commented Jan 30, 2024

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

Adds the code @siddharthkp created for his PR to solve this problem. When you scroll within a parent element that isn't the main body element, the Anchor now updates.

Original functionality

A video showing a panel of buttons, the user opens an overlay showing a cat by pressing one of the buttons. When the user scrolls the panel, the overlay cat remains fixed in place.

Original.functionality.mov

New functionality

A video showing a panel of buttons, the user opens an overlay showing a cat by pressing one of the buttons. When the user scrolls the panel, the overlay follows the button that was pressed. There's an issue shows where the overlay moves position based on the clipping of the parent rect.

New.functionality.mov

Changelog

New

This adds a new useElementObserver to observe when the rect for an element changes.

Changed

Changes useAnchoredPosition and AnchoredOverlay to use the new hook.

Removed

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

I'm not totally happy with this but couldn't see a better way to do it. I removed the instanceof Element because if broke the gatsby SSR build (Element isn't available server side). I've tried to use the solutions suggested by StackOverflow however excluding the library from the build isn't possible (we have to exclude the whole component, but then the Javascript still gets build for every other component so it still fails - I think some work on src/hooks/index.ts might resolve this but that was a rabbit hole I didn't have time to explore) and trying to only load the code client-side was proving to be fiddly and hacky given the structure of the code. In the end, I opted for the simplest solution 🤷

Do we need to worry about the increase to the size of browser.*.js?

I had a look at the weird issue whereby the overlay jumps position when the scroll takes the anchor button into a position where updatePosition returns a different orientation. We can fix this, but it's a much larger lift (and I believe this issue also exists for other overlays which don't suffer from this problem). To fix it, we'll probably need to rework useAnchoredPosition so that updatePosition is only called when the overlay is first opened, subsequent changes should move the overlay based on the original orientation of the overlay (resetting when closed).

Finally: do we want to add this functionality to AutocompleteOverlay and/or SelectPanel?

Merge checklist

Copy link

changeset-bot bot commented Jan 30, 2024

🦋 Changeset detected

Latest commit: 0448c28

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

Copy link
Contributor

github-actions bot commented Jan 30, 2024

size-limit report 📦

Path Size
dist/browser.esm.js 138.5 KB (+22.33% 🔺)
dist/browser.umd.js 138.9 KB (+21.98% 🔺)

@owenniblock
Copy link
Contributor Author

The build error here might be due to the same issue as described on this StackOverflow post.

@github-actions github-actions bot temporarily deployed to storybook-preview-4195 February 1, 2024 10:01 Inactive
@owenniblock owenniblock changed the title Copy code from @siddharthkp's original PR Scroll AnchoredOverlay when it's inside a scrolling child of the body Feb 1, 2024
@owenniblock owenniblock marked this pull request as ready for review February 1, 2024 10:28
@owenniblock owenniblock requested a review from a team as a code owner February 1, 2024 10:28
@owenniblock
Copy link
Contributor Author

owenniblock commented Feb 1, 2024

🤔 It looks like the change has surfaced an axe error where there wasn't one previously. It looks like the only axe error on the AnchoredOverlay is due to the sentinel elements being focusable as well as aria-hidden (you can find this error with a test of this page with the overlay open).

Maybe this fix has made the AnchoredOverlay open correctly when initiallyOpen={true}

UPDATE: I was wrong, because it now updates the React state, it needs to run inside act - going to try that.

@owenniblock
Copy link
Contributor Author

Converting this back to a draft. Might put it down for now and take a look in spare moments next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant