-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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(sqllab): fix query results sorting #18666
fix(sqllab): fix query results sorting #18666
Conversation
/testenv up |
@yousoph Container image not yet published for this PR. Please try again when build is complete. |
@yousoph Ephemeral environment creation failed. Please check the Actions logs for details. |
@@ -114,8 +114,9 @@ interface FilterableTableProps { | |||
|
|||
interface FilterableTableState { | |||
sortBy?: string; | |||
sortDirection: SortDirectionType; | |||
sortDirection?: SortDirectionType; |
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.
why is this optional?
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.
It's because of the third unsorted state that I added. Before it was being set with an initial value of "DESC" but it would be unsorted because sortBy starts as undefined. On the first click it would be set to "ASC" and sortBy gets set to that columns name. So now I have it set as undefined/optional initially and then clicking will set it to "ASC" then "DESC" then back to undefined.
I don't really like using undefined like that and I prefer to use null but the in the react-virtualized typing for SortIndicator and Table they have sortDirection set as optional. So if i used null I would have to do a fallback to undefined anyways because of their typing
fitted: false, | ||
displayedList: [...this.list], |
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.
is this the entirety of the data?
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.
Should be all of it. The data gets formatted and set to the list
variable in the constructor
constructor(props: FilterableTableProps) {
super(props);
this.list = this.formatTableData(props.data);
That's all the data that needs to be sorted from what I can tell. Lemme know if you think I missed something though
@@ -295,7 +296,30 @@ export default class FilterableTable extends PureComponent< | |||
sortBy: string; | |||
sortDirection: SortDirectionType; | |||
}) { | |||
this.setState({ sortBy, sortDirection }); | |||
this.setState( |
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 a lot to put in a setState, could we make the sort direction into its own function and pass it in here?
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.
We probably can. What do you mean by sort direction though? I am only seeing the sortDirection
param for the sort method and the prevSortDirection
param in the setState callback. I am not sure how to turn either into its own function though
This looks really good, I had some basic questions and one suggestion. |
Codecov Report
@@ Coverage Diff @@
## master #18666 +/- ##
==========================================
- Coverage 66.29% 66.24% -0.05%
==========================================
Files 1603 1604 +1
Lines 62744 62787 +43
Branches 6320 6336 +16
==========================================
- Hits 41593 41591 -2
- Misses 19499 19545 +46
+ Partials 1652 1651 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up |
@yousoph Ephemeral environment spinning up at http://34.219.227.216: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.
Manual tested it and it looks good!
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
Sorting
In SQL Lab there is currently no sorting for data with more than 50 columns because
FilterableTable
switches from using the react-virtualizedTable
to the react-virtualizedGrid
. Grid isn't set up to have sorting the way that Table is.The UI is the same as far as the user can tell so the sorting appears to stop working when there are too many columns.
I added listeners to the grid headers and added a
displayedList
state that gets sorted and used by the grid cells when rendering. I used theSortIndicator
icon component and used the samesortBy
andsortDirection
state that theTable
uses. They are never rendered at the same time so it didn't feel necessary to give the Grid its own separate states.For the same reason I slightly changed the
renderTable
method to use the newdisplayedList
state as well.This allows the initial list to remain intact because it is now doing "out of place" sorting instead of "in place" sorting. This allowed me to add a third cleared sort state that reverts the sorting back to before it was sorted.
Third Sort State (Cleared Sort State)
Added a third sorting state to FilterableTable that will return the results to its original order. As far as I know, this was not requested anywhere but I added it because I was in the area.
This is done by having an initial list and a displayed list and only mutating the displayed list when sorting. Then we just set the list back to its initial state when we have already sorted by descending and ascending.
The logic and flow is simple enough
The current sorting flow is:
This PR will change it to:
Logic:
Extra Padding
Increases the constant for the default padding on cells from
40
to50
The sorting icon was being truncated on some cells so I added a little more padded to fix it.
Screenshots
BEFORE
Example of clicking on headers in table with >50 columns
Example of sorting on tables <50 columns
Examples of headers being truncated after triggering sorting
AFTER
Example of clicking on headers in table with >50 columns
Examples of headers not being truncated after triggering sorting
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION