-
Notifications
You must be signed in to change notification settings - Fork 14k
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(CRUD views): change empty rows layout in TableCollection to be consistent with its headers #17515
fix(CRUD views): change empty rows layout in TableCollection to be consistent with its headers #17515
Conversation
…ading rows length to 12, remove width logic on empty loading columns
Codecov Report
@@ Coverage Diff @@
## master #17515 +/- ##
=======================================
Coverage 76.99% 76.99%
=======================================
Files 1046 1046
Lines 56491 56489 -2
Branches 7798 7798
=======================================
- Hits 43494 43493 -1
+ Misses 12741 12740 -1
Partials 256 256
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Thanks for the fix @corbinrobb ! I've tagged a few committers familiar with this area of the application for a review. |
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.
This is great! Can you add a screenshot of the favorite icon column with charts populated so I can see how it looks with the icons now that it's larger?
Thanks! @lyndsiWilliams Here are the screenshots of the difference that you asked for. Before is with Before with icons:After with icons:Before loading with no rows:After loading with no rows: |
This looks great! |
/testenv up |
@rusackas Ephemeral environment spinning up at http://35.166.245.17:8080. Credentials are |
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.
Love it! Filed from AND fixed in Denver! That has to be a first. Thank you for the contribution, and hit me up if you'd ever like to talk about contributing to Superset over a beer :)
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.
Thanks for the screenshots, it looks great!
Ephemeral environment shutdown and build artifacts deleted. |
Tested it on an ephemeral environment, and all seemed much improved! |
Thanks everyone! @rusackas I'm happy to help out. I may have to take you up on that offer! We can talk about Superset and how great Denver is. I will send you a message about it on slack sometime! |
…ading rows length to 12, remove width logic on empty loading columns (#17515) Co-authored-by: Corbin Robb <corbin@Corbins-MacBook-Pro.local>
SUMMARY
The headers were not lining up with the loading layout for when the chart list was empty while they were loading, causing a noticeable jump in size. Very noticeable when pressing any of the sort buttons at the top of the charts table while the table is empty. As described in issue #15127
TL/DR:
Empty state table cells were being rendered with text that gave the loading bars different widths than what the headers have when rows are empty.
The loading bar columns were hard set on
width
instead ofmin-width
like the headers, causing slight differences.Fixed by removing table cell text, removing logic setting hard width on empty columns, adding a class to make them inline-block, 100% width, and 1.2em height. Letting the loading bars stretch to be the size of the columns.
Other changes were changing the empty state rows to be 12 rows instead of 25 and changing Favorite column size from
'xs'
to'sm'
DETAILED DESCRIPTION:
What was happening was the loading bars that get rendered when the rows are empty and the loading state is true, were having their width being hard set to the size that the columns are. This is what we want when the rows aren't empty but when they are empty having the table headers be set on
min-width
and the columns being set onwidth
, was causing some issues.Another problem is that the loading bar CSS effect covers up the text that is within the columns, which is very nice when you sort a table that has rows with data in it. But the temporary rows and columns that act as the loading bars were being created with the text LOADING in every table cell. This is why it was the loading bars were short and why the column that holds the Favorite star icon was expanding by the length of the word LOADING.
The combination of the headers using
min-width
, the columns being set onwidth
, and the text "LOADING" being in a<span>
(inline) within the columns is what caused the jump in size and the look of the headers being pushed to the right of the screen when the empty/loading state appeared.THE FIX:
The fix I went with, is to remove the set width on the columns for the empty/loading table, letting the columns be the same size as their headers. Then create a new class for them that makes the
<span>
inside them inline-block instead of inline, sets width to 100% to fill the whole column, and adds a little height to better match text-filled size. Then remove the loading span, add an aria-label for the progressbar role tag because the text inside the span is no longer acting as the label.After that, the columns and loading bars are the length of the headers and a jump in size no longer occurs.
OTHER CHANGES:
I also changed the size of the Favorite Icon column to be
'sm'
(50px) instead of'xs'
(25px) because the icons are always about 50px wide.I changed the number of rows for the empty/loading table to better match with the size of the container around the 'No Data' graphic that gets displayed after loading is finished and there are no rows.
IN CONCLUSION:
I went with what I felt looked the best, made the most sense, and was the least intrusive to the code.
If the design and/or the implementation is not giving the desired effect or look, then please let me know so I can change it up.
BEFORE:
AFTER:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION