-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Fixed menu position based on scrollable parent #3830
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset is good to goLatest commit: 12a1744 We got this. This PR includes changesets to release 2 packages
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 |
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 doesn't seem to be fixing the issue on my end.
From what I can see part of the issue is in utils/getScrollParent()
. The criteria for selecting is incorrect and keeps returning the documentElement
.
Examining the method:
react-select/packages/react-select/src/utils.js
Lines 128 to 147 in 54a4dfb
export function getScrollParent(element: ElementRef<*>): Element { | |
let style = getComputedStyle(element); | |
const excludeStaticParent = style.position === 'absolute'; | |
const overflowRx = /(auto|scroll)/; | |
const docEl = ((document.documentElement: any): Element); // suck it, flow... | |
if (style.position === 'fixed') return docEl; | |
for (let parent = element; (parent = parent.parentElement); ) { | |
style = getComputedStyle(parent); | |
if (excludeStaticParent && style.position === 'static') { | |
continue; | |
} | |
if (overflowRx.test(style.overflow + style.overflowY + style.overflowX)) { | |
return parent; | |
} | |
} | |
return docEl; | |
} |
It becomes apparent why. The scroll parent could potentially be statically positioned.
The problem
I have a setup like this:
<FixedPosition >
<StaticScrollContainer>
<RelativePosition>
<ReactSelect />
</RelativePosition>
</StaticScrollContainer>
</FixedPosition>
A workaround
After adding position: relative
onto my scroll container w/ this PR fixes the issue.
An actual fix
We should update the logic of getScrollParent()
to set excludeStaticParent
to false
once we've reached our first non-static parent.
I could open a separate PR, but it might just be easier to include in your branch here (since it won't actually improve anything on it's own).
Conclusion
This combined with proper detection of the scroll parent works well!
@charrondev yeah I forgot to mention that we have to add I'll try to update the logic of |
@charrondev I've added the changes to |
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 is a major improvement on my end! Tested locally in our project https://github.com/vanilla/vanilla.
@JedWatson what needs to be done to proceed forward w/ this?
I also opened a PR with a fix and tests for this issue months ago, and it got no attention from the maintainers. I'd be curious to compare our two approaches, but I don't really have the time or energy right now. #3669 |
If @JedWatson is unavailable is there another active maintainer that could take a look at this? Going by the commits, the next person seems to be @gwyneplaine |
@flexdinesh I saw that you have started to look into stuff in this repo, so, whenever you get some time, can you please take a look at this PR or ask someone else to merge this? Would really appreciate your help. Thanks! |
Added scrollParentTop in calculation of viewSpaceBelow and scrollDown
This fixes 3822, as previously the positioning of the menu was based on the window height and the menu was being truncated if it could fit inside the window but not the scrollable parent.
But now, the position will be based on the height and boundaries of the closest scrollable parent element, like in the case of a slide panel or a dialog box, where the panel/dialog can be placed anywhere in the window which limits the area where the menu can be viewed.
-----UPDATE-----
The scrollable parent must have a 'non-static' (or default) position in order for this fix to work. Ideally,
position: relative
would be enough for most cases but can be anything other thanstatic
.