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

[useAnchorPositioning] Add OffsetFunction for sideOffset and alignOffset #1223

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

atomiks
Copy link
Contributor

@atomiks atomiks commented Dec 24, 2024

Closes #1195

@atomiks atomiks added component: select This is the name of the generic UI component, not the React module! component: menu This is the name of the generic UI component, not the React module! component: tooltip This is the name of the generic UI component, not the React module! component: popover The React component. component: preview card The React component. labels Dec 24, 2024
@mui-bot
Copy link

mui-bot commented Dec 24, 2024

Netlify deploy preview

https://deploy-preview-1223--base-ui.netlify.app/

Generated by 🚫 dangerJS against a7c255d

Comment on lines 125 to 147
// Keep these reactive if they're not functions
const sideOffsetDep = typeof sideOffset !== 'function' ? sideOffset : 0;
const alignOffsetDep = typeof alignOffset !== 'function' ? alignOffset : 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Positioning reactivity is lost when they specify a function. We can't tell if they're going to React.useCallback or not - usually the function gets inlined like:

<Popover.Positioner sideOffset={(sizes) => sizes.anchor.height / 2}>

So it can't be specified as a dependency as it's always changing. This means if they update the math inside the function, the position won't update live on fast refresh (but it will whenever the anchoring updates at some point)

Copy link
Member

Choose a reason for hiding this comment

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

If this ever becomes a problem, the sideOffset's argument can include an update callback function that will force updating the layout.

Comment on lines +21 to +23
"type": "number | (data) => number",
"default": "0",
"description": "Distance between the anchor and the popup."
"description": "Distance between the anchor and the popup in pixels.\nAlso accepts a function that returns a number to read the dimensions of the anchor and popup,\nalong with its side and alignment.\n\n- `data.anchor`: the dimensions of the anchor element with properties `width` and `height`.\n- `data.popup`: the dimensions of the popup element with properties `width` and `height`.\n- `data.side`: which side of the anchor element the popup is aligned against.\n- `data.align`: how the popup is aligned relative to the specified side."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-12-24 at 7 24 35 pm

side: getLogicalSide(sideParam, getSide(currentPlacement), isRtl),
align: getAlignment(currentPlacement) || 'center',
anchor: { width: rects.reference.width, height: rects.reference.height },
popup: { width: rects.floating.width, height: rects.floating.height },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically this is the Positioner element, not Popup, but in practically every case they should be the same since it's a presentational wrapper

@atomiks atomiks marked this pull request as ready for review December 26, 2024 03:43
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 7, 2025
@atomiks atomiks force-pushed the feat/offset-functions branch from 5270eaa to a7c255d Compare January 7, 2025 11:07
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 7, 2025
@michaldudak
Copy link
Member

Is it possible to place a popup outside of the trigger's bounds?
I'm trying to align the start of the popup with the right-hand side of the trigger with some margin, but it seems that the offset can only be as high as the anchor width (the + 5 is ignored):

<Popover.Positioner
  sideOffset={5}
  alignOffset={(sizes) => sizes.anchor.width + 5}
  align="start"
>

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 8, 2025
@atomiks
Copy link
Contributor Author

atomiks commented Jan 8, 2025

@michaldudak the limitShift logic forcefully prevents that from happening so it doesn't "detach". I'm not sure if it's important to support that in practice

@michaldudak
Copy link
Member

OK, it's fine. We could write a few words about it in the docs, so it's not surprising. I miss having a dedicated section per prop/group of props

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: menu This is the name of the generic UI component, not the React module! component: popover The React component. component: preview card The React component. component: select This is the name of the generic UI component, not the React module! component: tooltip This is the name of the generic UI component, not the React module! PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Popover] [Potential Feature Request] How to align the start of the popover with the center of the trigger?
3 participants