-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,8 +114,9 @@ interface FilterableTableProps { | |
|
||
interface FilterableTableState { | ||
sortBy?: string; | ||
sortDirection: SortDirectionType; | ||
sortDirection?: SortDirectionType; | ||
fitted: boolean; | ||
displayedList: Datum[]; | ||
} | ||
|
||
export default class FilterableTable extends PureComponent< | ||
|
@@ -175,8 +176,8 @@ export default class FilterableTable extends PureComponent< | |
this.totalTableHeight = props.height; | ||
|
||
this.state = { | ||
sortDirection: SortDirection.ASC, | ||
fitted: false, | ||
displayedList: [...this.list], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 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 |
||
}; | ||
|
||
this.container = React.createRef(); | ||
|
@@ -191,7 +192,7 @@ export default class FilterableTable extends PureComponent< | |
} | ||
|
||
getWidthsForColumns() { | ||
const PADDING = 40; // accounts for cell padding and width of sorting icon | ||
const PADDING = 50; // accounts for cell padding and width of sorting icon | ||
const widthsByColumnKey = {}; | ||
const cellContent = ([] as string[]).concat( | ||
...this.props.orderedColumnKeys.map(key => { | ||
|
@@ -295,7 +296,31 @@ export default class FilterableTable extends PureComponent< | |
sortBy: string; | ||
sortDirection: SortDirectionType; | ||
}) { | ||
this.setState({ sortBy, sortDirection }); | ||
let updatedState: FilterableTableState; | ||
|
||
const shouldClearSort = | ||
this.state.sortDirection === SortDirection.DESC && | ||
this.state.sortBy === sortBy; | ||
|
||
if (shouldClearSort) { | ||
updatedState = { | ||
...this.state, | ||
sortBy: undefined, | ||
sortDirection: undefined, | ||
displayedList: [...this.list], | ||
}; | ||
} else { | ||
updatedState = { | ||
...this.state, | ||
sortBy, | ||
sortDirection, | ||
displayedList: [...this.list].sort( | ||
this.sortResults(sortBy, sortDirection === SortDirection.DESC), | ||
), | ||
}; | ||
} | ||
|
||
this.setState(updatedState); | ||
} | ||
|
||
fitTableToWidthIfNeeded() { | ||
|
@@ -362,6 +387,17 @@ export default class FilterableTable extends PureComponent< | |
}; | ||
} | ||
|
||
sortGrid = (label: string) => { | ||
this.sort({ | ||
sortBy: label, | ||
sortDirection: | ||
this.state.sortDirection === SortDirection.DESC || | ||
this.state.sortBy !== label | ||
? SortDirection.ASC | ||
: SortDirection.DESC, | ||
}); | ||
}; | ||
|
||
renderTableHeader({ | ||
dataKey, | ||
label, | ||
|
@@ -425,8 +461,14 @@ export default class FilterableTable extends PureComponent< | |
: style.top, | ||
}} | ||
className={`${className} grid-cell grid-header-cell`} | ||
role="columnheader" | ||
tabIndex={columnIndex} | ||
onClick={() => this.sortGrid(label)} | ||
> | ||
<div>{label}</div> | ||
{label} | ||
{this.state.sortBy === label && ( | ||
<SortIndicator sortDirection={this.state.sortDirection} /> | ||
)} | ||
</div> | ||
</Tooltip> | ||
); | ||
|
@@ -444,7 +486,7 @@ export default class FilterableTable extends PureComponent< | |
style: React.CSSProperties; | ||
}) { | ||
const columnKey = this.props.orderedColumnKeys[columnIndex]; | ||
const cellData = this.list[rowIndex][columnKey]; | ||
const cellData = this.state.displayedList[rowIndex][columnKey]; | ||
const cellText = this.getCellContent({ cellData, columnKey }); | ||
const content = | ||
cellData === null ? <i className="text-muted">{cellText}</i> : cellText; | ||
|
@@ -563,19 +605,13 @@ export default class FilterableTable extends PureComponent< | |
rowHeight, | ||
} = this.props; | ||
|
||
let sortedAndFilteredList = this.list; | ||
let sortedAndFilteredList = this.state.displayedList; | ||
// filter list | ||
if (filterText) { | ||
sortedAndFilteredList = this.list.filter((row: Datum) => | ||
sortedAndFilteredList = sortedAndFilteredList.filter((row: Datum) => | ||
this.hasMatch(filterText, row), | ||
); | ||
} | ||
// sort list | ||
if (sortBy) { | ||
sortedAndFilteredList = sortedAndFilteredList.sort( | ||
this.sortResults(sortBy, sortDirection === SortDirection.DESC), | ||
); | ||
} | ||
|
||
let { height } = this.props; | ||
let totalTableHeight = height; | ||
|
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