-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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(Explore): Cell height and spacing for Data panel #15821
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15821 +/- ##
=======================================
Coverage 77.05% 77.06%
=======================================
Files 984 984
Lines 51650 51669 +19
Branches 6991 6995 +4
=======================================
+ Hits 39801 39819 +18
- Misses 11625 11626 +1
Partials 224 224
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@geido Thank you for the PR! Diego, the purpose of the change is 1) visual consistency for all tables, 2) show more rows in the south table. Is there a particular reason why we are only showing 4 rows per page now after the spacing is reduced? original comment cc @kgabryje |
/testenv up |
@junlincc Ephemeral environment spinning up at http://34.217.84.207:8080. Credentials are |
@junlincc the PR only makes the table style in the data panel consistent with the table chart. The number of the shown items depends on the screen size. The screenshot after the PR was taken on a 15' screen. Probably the screenshot in the original issue (which I have re-attached to this PR as a reference) was taken with a larger screen. |
P.S. I have closed the downloads in my browser which were occupying some space and I am seeing 5 items on my 15' laptop now. If we want to show more rows we might need to act also on the height of the datapanel itself as reducing the height of the cells does not give that much additional space. I would also like to point out that there are several other visual differences between the two tables. On the table chart, in every other row, the cells have a different background color. Also, the pagination is right positioned and comes in different colors compared to the table in the datapanel one. Finally, the table of the datapanel is used in other places too, such as the "Change dataset" modal. I left that one untouched as the issue did not specify to change it everywhere. If we are looking to make the tables look consistent (aka the same) everywhere, we have to change several other layout parts. CC @junlincc |
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.
code looks good to me.
LGTM! Thanks so much for making Superset SLICK! |
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.
Ephemeral environment shutdown and build artifacts deleted. |
Nice! Thanks for the improvement ❤️ |
* Fix height and spacing * Remove unnecessary padding
* Fix height and spacing * Remove unnecessary padding
* Fix height and spacing * Remove unnecessary padding
SUMMARY
Makes the height of the cells consistent and improves spacing.
Fixes: #15691
BEFORE
AFTER
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION