-
Notifications
You must be signed in to change notification settings - Fork 529
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
Overlay and its composing components take a portalContainerName prop #1388
Conversation
🦋 Changeset detectedLatest commit: f900777 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
Could you explain why the |
Good question! Today where our overlays are anchored to an element inside a scrolling element, the overlay does not track with that anchor. Allowing components to specify their portal container gives us overlays rendered as a descendant of the scrolling element, so they track with their anchor. @colebemis if this makes sense to you, I'll copy it to the description. |
@jfuchs That makes sense! Can you add that to the PR description and the changeset? |
306c002
to
22b5669
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the right approach, but I think we can simplify this quite a bit by leveraging the existing props
docs/content/ActionMenu.mdx
Outdated
| onAction | (props: ItemProps) => void | `undefined` | Optional. If defined, this function will be called when a menu item is activated either by a click or a keyboard press. | | ||
| open | boolean | `undefined` | Optional. If defined, ActionMenu will use this to control the open/closed state of the Overlay instead of controlling the state internally. Should be used in conjunction with the `setOpen` prop. | | ||
| setOpen | (state: boolean) => void | `undefined` | Optional. If defined, ActionMenu will use this to control the open/closed state of the Overlay instead of controlling the state internally. Should be used in conjunction with the `open` prop. | | ||
| portalContainerName | boolean | `undefined` | Optional. See the `portalContainerName` prop of `Overlay` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this type is wrong (in each of the docs). Should be string
if (scrollingElementRef.current && includePortal) { | ||
registerPortalRoot(scrollingElementRef.current) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This story doesn't appear to use portalContainerName
. Instead, it looks like it just registers the default portal within the scrolling element
{...(portalContainerName ? {portalContainerName} : {})} | ||
{...overlayProps} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have an option to pass any overlayProps
through the Anchored Overlays, which seems like an OK path for this option as well. It should be rarely used, kind of like height/width which use this other approach. Should significantly cut down on the surface area of this PR as well
5b429c2
to
37e7b7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much cleaner, thanks for simplifying and updating the stories! One small item then this should be good to go
src/Overlay.tsx
Outdated
@@ -15,6 +15,7 @@ type StyledOverlayProps = { | |||
maxHeight?: keyof Omit<typeof heightMap, 'auto' | 'initial'> | |||
visibility?: 'visible' | 'hidden' | |||
anchorSide?: AnchorSide | |||
portalContainerName?: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only need to define this prop in OverlayProps
. The values here are specifically for what gets passed in to the styled component. You should be able to remove this line without issues
6a780c7
to
37e7b7a
Compare
This change adds a
portalContainerName
prop toOverlay
,AnchoredOverlay
,DropdownMenu
,SelectPanel
, andActionMenu
. This allows overlays with an anchor inside a scrolling container to track with their anchor, so long as the specified portal is also inside that scrolling container.Closes https://github.com/github/primer/issues/229.
Screenshots
Screen.Recording.2021-08-23.at.2.23.55.PM.mov
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.