-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix primary key select #16937
Fix primary key select #16937
Conversation
…flicts with what we're trying to do
I tested this with #16934 merged in and did not see any regression. |
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.
LGTM! 😉
}} | ||
backspaceRemovesValue={false} | ||
controlShouldRenderValue={false} | ||
hideSelectedOptions={false} | ||
isClearable={false} | ||
menuIsOpen={isOpen} | ||
// menuPosition={"fixed"} | ||
menuShouldBlockScroll={false} |
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.
Yes, it will fix the issue, but on the other side, it could create a new bug with floating menu
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.
A later issue! 😂
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.
There's a form validation bug that appears when a user picks a primary key from the dropdown. It does not freeze any longer, but I think we should solve that also.
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.
Tested locally. It fixes the original issue. Also briefly looked to see if we ended up with displaced menus on scroll with this change. I didn't see any... but it was only a quick look as this is a good fix to get in sooner than later.
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.
Tested locally. It fixes the original issue. Also briefly looked to see if we ended up with displaced menus on scroll with this change. I didn't see any... but it was only a quick look as this is a good fix to get in sooner than later.
* Scroll blocking adds a second "cover everything" dom element that conflicts with what we're trying to do * Remove commented code * Fixing error validation message
* Scroll blocking adds a second "cover everything" dom element that conflicts with what we're trying to do * Remove commented code * Fixing error validation message
What
Fix primary key selection on replication view
Resolves https://github.com/airbytehq/oncall/issues/634
How
The DropDown component adds a complete-screen DOM element to prevent scrolling which conflicted with our "click outside to exit" DOM element. By telling the DropDown not to block scrolling it removes the conflict.
Playing with the z-index did not yield fruitful results.
🚨 User Impact 🚨
This affects dropdowns so there's always a chance for regression.
#16934 is in the same space so it will be important to test both throughly!