fix(components): Adding optional strategy prop to popovers to fix scrolling layout issue #2595
Conversation
- Introduced a new optional `strategy` prop to the Popover component, allowing users to specify the positioning strategy ('absolute' or 'fixed').
- This is used to fix an issue where the popover, when scrolled out of the screen in the side nav, causes the page to scroll further than desired
- Updated PopoverProvider and usePopover hooks to support the new strategy prop.
Deploying atlantis with
|
| Latest commit: |
1e50bfd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d2f79926.atlantis.pages.dev |
| Branch Preview URL: | https://job-126928-popoverscrolling.atlantis.pages.dev |
| * The strategy to use for the Popover. | ||
| * @default 'absolute' | ||
| */ | ||
| readonly strategy?: "absolute" | "fixed"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😄
|
Could not publish Pre-release for 1e50bfd. View Logs |
| boundary: "viewport", | ||
| padding: 8, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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.
There was a problem hiding this comment.
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
|
I'm having trouble reproducing the happy path with this solution I've packaged the local version of this branch, cleared all caches and installed it in product, then applied the I did force the popups to show but I can't imagine that having any real effect on what I'm trying to test out here. update: nevermind I see the problem. it's not actually running the new code I'm somehow in a state where the types/props have updated and accept |
|
something I noticed we'll want to add is in the non-composed since we're (rightfully) defining the that way the non-composed invocations of |
Motivations
There is an issue with the popovers when they scroll outside of the viewport in the sidenav, as it leads to the screen being larger and you can scroll down to this weird white space.
Changes
Introduced a new optional
strategyprop to the Popover component, allowing users to specify the positioning strategy ('absolute' or 'fixed').Updated PopoverProvider and usePopover hooks to support the new strategy prop.
Also added some padding so the popover wouldn't sit right on the bottom of the page.
Screen.Recording.2025-06-20.at.2.51.49.PM.mov
Added
Changed
Deprecated
Removed
Fixed
This is the issue that's being fixed
Screen.Recording.2025-06-20.at.3.49.01.PM.mov
Security
Testing
Changes can be
tested via Pre-release
In Atlantis we use Github's built in pull request reviews.