Fix for Menu bottom is scrolled into view even when menuShouldScrollIntoView=false #3262
Conversation
torkelo
force-pushed
the
menu-scroll-into-view-issue
branch
from
6a556bd
to
f4de0ea
Compare
|
rebased with master and created a bug report issue #3459 |
|
Any chance to merge this bugfix? |
|
Why is this PR can't be merged? Have been waiting for many days! |
|
I've also had a large number of users experiencing this issue, and it's a really huge UX issue since it's unexpected for the container to be moving around. It would be very nice to get this merged! |
|
The same. I'm waiting for this changes. |
|
@gwyneplaine could you take a look on the PR? |
|
Please! |
|
Almost a year !!! |
|
@JedWatson @gwyneplaine Please have a look at this PR, thanks |
…IntoView is false, happens when parent container is a scrollable div further down the page. The bug casues the menu bottom to be scrolled into view so that the select input/search is not visible
torkelo
force-pushed
the
menu-scroll-into-view-issue
branch
from
f4de0ea
to
f2412a4
Compare
🦋 Changeset detectedLatest commit: 7a414a7 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 |
|
Rebased this against latest master |
|
Please try to find time to merge this, simple fix to a bug that forces us to maintain an npm fork. |
|
I know it's hard to find time to review PRs but this seems like an easy one to review |
|
If anyone needs this fix: https://www.npmjs.com/package/@torkelo/react-select |
|
@JedWatson friendly ping :) |
bladey
added
uncertain
pr/bug-fix
bladey
added
the
pr/needs-review
|
Hey @torkelo, could you please provide a CodeSandbox of this PR with a working example? I'd like to test and get this merged. |
bladey
added
awaiting-author-response
|
Not sure I will have time this week. You mean a CodeSandbox that replicates the issue or one that includes this change? |
|
You can try this change on https://play.grafana.org/d/000000012/grafana-play-home?editPanel=2&orgId=1
|
|
yay! can remove my npm fork after this :) |
|
@JedWatson Looks like this is super close to being merged! I'd love to have this fix ❤️ |
bladey
added
pr/in-review
|
EDIT: 🎉 I've archived the fork now that we've got some momentum in this repo and Jed is involved again. Sorry for the disturbance! |
|
Echoing everyone who has advocated for this fix being merged 😄 This would be super helpful. |
ebonow
added
the
menu-bug
|
Really looking forward to this merge, thank you for awesome lib! 🙏 |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7a414a7:
|
|
Sorry everyone for the long wait. This seems like an obvious bug fix and will move this into consideration for next version release. |
|
@freewind (or anyone else) have any reservations about this? |
|
@ebonow Sorry, just have been waiting too long and feel very sad |
@freewind I can understand that and I empathize. I feel bad that so many people have given their time to contribute and then didn't even get the courtesy of a response. It's exactly why myself and others are putting in so much of our own free time to work through everything, and we're doing our best to make it right. And while just last year there were 1,452 issues closed, 151 PR's closed, 56 PR's merged, communication could have been much better, so it's something that's been made a priority. As such, please feel free to use the discussion area or subscribe to #4293 for updates on releases if that's something that interests you. That all said, I am committed to working through every issue and PR and any help with that is welcome. |
|
Hi everyone, we finally got to this PR in our review this week, and it has now been approved and will be released imminently. I want to say I'm sorry for leaving this so long. It's heartbreaking, and I apologise. It's such a simple fix to a clear bug that unfortunately just got lost in the noise for me. @ebonow @Methuselah96 @Rall3n and @bladey have my immense gratitude for helping get the project back under control from a maintenance perspective. As Eric said, it's been an epic undertaking 🙏 |
When the menu is placed inside a scrollable div a bit down on the page the menu always triggers a scroll to bottom of this div even when menuShouldScrollIntoView=false. This is really a big issue as it scrolls the parent container so much so the react select input does not become visible.
Was fixed by adding the if (shouldScroll) to the last case in getMenuPlacement. This case did not check this bool flag, the other scenarios in this case switch do check this variable.
Fixes #3459