-
Notifications
You must be signed in to change notification settings - Fork 14.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
fix(cosmetic): Fix Datasource Modal Out Of Box #20237
Conversation
Thanks for the contribution! Actually this css snippet can be found in the context of this issue, so we probably can't remove it easily, but if we verify that the issue is working correctly, I think it's okay to remove it. I think the problem is probably related to this issue, which is about maxHeight of the modal. cc @rusackas |
Thanks for reviewing. |
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.
Approving since this works for the modal in question. As per @stephenLYZ 's point, this might indeed be an issue on other modals. I'm on the fence between just reverting mine to minimize my PR to minimize fallout, or taking this PR and attempting to do a quick audit and fix-forward the others that might be affected.
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! Thanks for the work!
FWIW, when I run this locally, I'm not seeing the issue that @AAfghahi was tackling in his earlier PR, but I'm spinning up an ephemeral environment, and pinging him for a review, just in case :) |
/testenv up |
@rusackas Ephemeral environment spinning up at http://34.221.129.77:8080. Credentials are |
Looked at the other two modals that allow overflow, and they don't seem so problematic. I think this fix is the better way forward than the revert, but I'll let it simmer for a moment for discussion. |
Codecov Report
@@ Coverage Diff @@
## master #20237 +/- ##
=======================================
Coverage 66.50% 66.50%
=======================================
Files 1726 1726
Lines 64790 64790
Branches 6829 6829
=======================================
Hits 43087 43087
Misses 19967 19967
Partials 1736 1736
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Ephemeral environment shutdown and build artifacts deleted. |
Thanks for fixing this. The Save Chart modal has overflow visible, but looks OK since it's short. Same with the Dashboard's Refresh Interval Modal. We can do this same fix on either, but it makes matters worse on the Refresh Interval modal. If you do this fix on the Save Chart modal, it works fine, but looks a bit goofy as the bottom Select menu goes upward to cover the Input above it. I'll leave them both alone, but I'm open to ideas/consideration. |
SUMMARY
remove 'overflow: visible' from DatasourceModal component, to fit with the height limit, and show the scrollbar.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
After
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION