Skip to content

fix(components): Adding optional strategy prop to popovers to fix scrolling layout issue #2595

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

Closed
wants to merge 2 commits into from
Closed

fix(components): Adding optional strategy prop to popovers to fix scrolling layout issue #2595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
beb5aa7
Adding optional strategy prop to popovers to fix scrolling layout issue
ethanmillard Jun 20, 2025
1e50bfd
generated json file
ethanmillard Jun 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/components/src/Popover/Popover.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ export interface PopoverProps {
*/
readonly open: boolean;

/**
* The strategy to use for the Popover.
* @default 'absolute'
*/
readonly strategy?: "absolute" | "fixed";
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll have to play around with this a bit to make sure I'm understanding the exact difference in behavior, unless it's a direct 1:1 with CSS but it makes me wonder when we'd even want to use absolute if things will always get weird with scrolling and items moving

I feel like you'd always want your tooltip to be beside the element you specified?

that said, it could/would be a pretty significant change to existing tooltips if we swapped the default to fixed - so having an opt-in prop is for sure the safer option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @ZakaryH

and ya, I had the same thoughts, but the idea of changing the default for everyone was a little terrifying lol and I don't know react-popper well enough. Which is why I opted to just allow the option

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough! I agree this is the safer approach. plus we have an upcoming ticket to replace the deprecated react-popper with floating-ui though they will support the exact same prop

https://floating-ui.com/docs/usefloating#strategy

sounds like defaulting to absolute is the right thing to do for most layouts, and fixed is more of an exception which matches what we're doing 😄


/**
* Callback executed when the user wants to close/dismiss the Popover
*/
Expand Down Expand Up @@ -54,7 +60,10 @@ export interface PopoverProps {
}

export type PopoverProviderProps = PropsWithChildren<
Pick<PopoverProps, "preferredPlacement" | "attachTo" | "open"> & {
Pick<
PopoverProps,
"preferredPlacement" | "attachTo" | "open" | "strategy"
> & {
/**
* **Use at your own risk:** Custom class names for specific elements. This should only be used as a
* **last resort**. Using this may result in unexpected side effects.
Expand Down
2 changes: 2 additions & 0 deletions packages/components/src/Popover/PopoverContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,14 @@ export function PopoverProvider({
open,
UNSAFE_className,
UNSAFE_style,
strategy,
}: PopoverProviderProps) {
const { setPopperElement, setArrowElement, popperStyles, attributes } =
usePopover({
preferredPlacement,
attachTo,
open,
strategy,
});

if (!open) return null;
Expand Down
21 changes: 19 additions & 2 deletions packages/components/src/Popover/usePopover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ export const usePopover = ({
preferredPlacement,
attachTo,
open,
}: Pick<PopoverProps, "preferredPlacement" | "attachTo" | "open">) => {
strategy = "absolute",
}: Pick<
PopoverProps,
"preferredPlacement" | "attachTo" | "open" | "strategy"
>) => {
const [popperElement, setPopperElement] = useState<HTMLElement | null>();
const [arrowElement, setArrowElement] = useState<HTMLElement | null>();

Expand All @@ -23,6 +27,13 @@ export const usePopover = ({
offset: [0, 10],
},
},
{
name: "preventOverflow",
options: {
boundary: "viewport",
padding: 8,
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

couple thoughts on this change

first, we'd be applying it to Popovers across the board. I think I get what preventOverflow does, but I'd definitely like to make sure we're confident applying this to everything - otherwise it might also be something to opt in to.

second, I see 8 in the example from Popper but I wonder what that's based on. seems fairly arbitrary. I could see there being cases where you don't really mind your Popover getting very close to an edge.

oh one more is that if we did go ahead with this, it would be preferable to use a design spacing token like space-small (not necessarily that one, just an example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

those all make sense to me

should I make those changes?
or is this still in the decision phase of if this is the right path or not?

Copy link
Contributor

@ZakaryH ZakaryH Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is the right path to go overall, with the strategy prop and its default.

I'll try to play around with the preventOverflow a bit more. I haven't really been able to get it to behave how I expected, likely due to me not setting up the right scenario.

Update

so prevent overflow has a few different settings for the boundary for things like the viewport, container etc.

however since we are portalling the Popover to the body, they will have no difference because the viewport will be the same as the parent, given the parent is the document.body

it won't really matter what we provide as the boundary value

the actual padding value we give to preventOverflow will still work, it's just a bit strange because we have to remember it's always going to use the body as the element it will avoid overflowing
image

I don't think it's a bad value to have. it just might shift some Popovers around by whatever amount we choose if they are right up against the edge of the screen.

I would like to know how you are testing these changes in the frontend project though, I'm getting some really bizzarre results when I try to use a local version of these changes. seems like Vite is doing something unexpected.

then there's one other thing I noticed that I'll make a separate comment about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up just copying the entire popover folder into JFE, then fixed the couple of imports

I was able to play around with the changes really fast that way and just copied back everything I had done into atlantis

{
name: "flip",
options: {
Expand All @@ -38,11 +49,17 @@ export const usePopover = ({
{
modifiers,
placement: preferredPlacement,
strategy,
},
);
useRefocusOnActivator(open);

return { setPopperElement, setArrowElement, popperStyles, attributes };
return {
setPopperElement,
setArrowElement,
popperStyles,
attributes,
};
};

// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand Down
30 changes: 30 additions & 0 deletions packages/site/src/content/Popover/Popover.props.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.