-
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: south pane scrolling issues #12318
fix: south pane scrolling issues #12318
Conversation
cc @rusackas |
Codecov Report
@@ Coverage Diff @@
## master #12318 +/- ##
==========================================
- Coverage 67.11% 67.05% -0.06%
==========================================
Files 967 1001 +34
Lines 47508 49384 +1876
Branches 4632 5033 +401
==========================================
+ Hits 31884 33114 +1230
- Misses 15505 16142 +637
- Partials 119 128 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@@ -555,6 +560,7 @@ export default class FilterableTable extends PureComponent< | |||
style={{ height }} | |||
className="filterable-table-container" | |||
ref={this.container} | |||
css={filterableTableStyles} |
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.
the css prop approach works, but I kind of lean toward the styled.div
implementation like you did with StyledPane
. If you did that, you get the bonus win of being able to kill this off from superset-frontend/src/SqlLab/main.less
:
.filterable-table-container {
margin-top: 48px;
}
... and switch it to margin-top: ${({ theme }) => theme.gridUnit * 12}px;
for a bonus bonus win ;)
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.
ok, great. I didn't see that style in less, that's another win for sure. I was thinking something like if you can utilize the style, use styled.div, etc, otherwise css prop?
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.
In general, this looks like it does a fine job of fixing the acute issue and moving some LESS to Emotion (yay!). Since this is a rush
I'm fine with approving this, but I thought I'd point out a few things around theme variables... let me know if you would be able to get those in before merging.
Co-authored-by: Evan Rusackas <evan@preset.io>
Co-authored-by: Evan Rusackas <evan@preset.io>
Thanks for the review @rusackas! I addressed 3/4 and saved one for later. |
6f3f48d
to
346230a
Compare
@rusackas @betodealmeida @hughhhh @dpgaspar Can someone merge this in for tomorrow's branch cut? Thanks |
Thanks for the great work, @eschutho! |
Closes #12046 |
SUMMARY
Fixes scrolling issues where the tabs were overlapping the table and also where the last row was being cut off.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
southpane2.mov
TEST PLAN
manual visual testing.. all other tests should still pass
ADDITIONAL INFORMATION