Skip to content
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

Conversation

corbinrobb
Copy link
Contributor

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 of min-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 on width, 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 on width, 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:

BeforeGif

Screen Shot 2021-11-22 at 3 15 15 PM

AFTER:

AfterGif

Screen Shot 2021-11-22 at 3 11 25 PM

TESTING INSTRUCTIONS

  • Open Charts from the menu at top of the screen
  • Change the Charts filters until you have an empty chart table
  • Click away on those sorting headers
  • Observe the loading bars in all their magnificence

ADDITIONAL INFORMATION

…ading rows length to 12, remove width logic on empty loading columns
@codecov
Copy link

codecov bot commented Nov 23, 2021

Codecov Report

Merging #17515 (3946463) into master (1f8eff7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           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           
Flag Coverage Δ
javascript 71.05% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...perset-frontend/src/views/CRUD/chart/ChartList.tsx 74.35% <ø> (ø)
...-frontend/src/components/TableCollection/index.tsx 100.00% <100.00%> (ø)
...d/src/visualizations/TimeTable/FormattedNumber.jsx
...d/src/visualizations/TimeTable/FormattedNumber.tsx 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f8eff7...3946463. Read the comment docs.

@villebro
Copy link
Member

Thanks for the fix @corbinrobb ! I've tagged a few committers familiar with this area of the application for a review.

Copy link
Member

@lyndsiWilliams lyndsiWilliams left a 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?

@corbinrobb
Copy link
Contributor Author

Thanks! @lyndsiWilliams

Here are the screenshots of the difference that you asked for.

Before is with 'xs' and after is 'sm'

Before with icons:

Before Shot

After with icons:

After Shot

Before loading with no rows:

Screen Shot 2021-11-23 at 2 08 25 PM

After loading with no rows:

Screen Shot 2021-11-23 at 2 10 05 PM

@ghost
Copy link

ghost commented Nov 23, 2021

This looks great!

@rusackas
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@rusackas Ephemeral environment spinning up at http://35.166.245.17:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@rusackas rusackas left a 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 :)

Copy link
Member

@lyndsiWilliams lyndsiWilliams left a 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!

@rusackas rusackas merged commit c216565 into apache:master Nov 23, 2021
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@rusackas
Copy link
Member

Tested it on an ephemeral environment, and all seemed much improved!

@corbinrobb
Copy link
Contributor Author

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!

AAfghahi pushed a commit that referenced this pull request Jan 10, 2022
…ading rows length to 12, remove width logic on empty loading columns (#17515)

Co-authored-by: Corbin Robb <corbin@Corbins-MacBook-Pro.local>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants