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.

Sign up for GitHub

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

improve sorting when filtering datasets #2834

Merged
merged 3 commits into from
Jul 2, 2018
Merged

improve sorting when filtering datasets #2834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
68df245
improve sorting when filtering datasets
philippotto Jun 29, 2018
5fc5123
update snapshots
philippotto Jun 29, 2018
8f8d0ef
Merge branch 'master' into dice-sort-datasets
philippotto Jul 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 70 additions & 8 deletions app/assets/javascripts/dashboard/advanced_dataset/advanced_dataset_view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// @flow
/* eslint-disable jsx-a11y/href-no-hash, react/prefer-stateless-function */
/* eslint-disable jsx-a11y/href-no-hash, react/prefer-stateless-function, react/no-unused-state */

import * as React from "react";
import TemplateHelpers from "libs/template_helpers";
Expand All @@ -9,6 +9,8 @@ import DatasetActionView from "dashboard/advanced_dataset/dataset_action_view";
import DatasetAccessListView from "dashboard/advanced_dataset/dataset_access_list_view";
import type { DatasetType } from "dashboard/dataset_view";
import type { APITeamType } from "admin/api_flow_types";
import dice from "dice-coefficient";
import _ from "lodash";

const { Column } = Table;

Expand All @@ -19,27 +21,85 @@ type Props = {
searchQuery: string,
};

class AdvancedDatasetView extends React.PureComponent<Props> {
type State = {
prevSearchQuery: string,
sortedInfo: Object,
};

class AdvancedDatasetView extends React.PureComponent<Props, State> {
static getDerivedStateFromProps(nextProps: Props, prevState: State): $Shape<State> {
const maybeSortedInfo =
// Clear the sorting exactly when the search box is initially filled
// (searchQuery changes from empty string to non-empty string)
nextProps.searchQuery !== "" && prevState.prevSearchQuery === ""
? {
sortedInfo: { columnKey: null, order: "ascend" },
}
: {};

return {
prevSearchQuery: nextProps.searchQuery,
...maybeSortedInfo,
};
}

constructor() {
super();
this.state = {
sortedInfo: {
columnKey: "created",
order: "descend",
},
prevSearchQuery: "",
};
}

handleChange = (pagination: Object, filters: Object, sorter: Object) => {
this.setState({
sortedInfo: sorter,
});
};

render() {
const filteredDataSource = Utils.filterWithSearchQueryOR(
this.props.datasets,
["name", "description"],
this.props.searchQuery,
);

const { sortedInfo } = this.state;
const sortedDataSource =
// Sort using the dice coefficient if the table is not sorted otherwise
// and if the query is longer then 3 characters to avoid sorting *all* datasets
this.props.searchQuery.length > 3 && sortedInfo.columnKey == null
? _.chain(filteredDataSource)
.map(row => ({
row,
diceCoefficient: dice(row.name, this.props.searchQuery),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether this will result in strange results if the user searches for a word in the description. The search results are filtered using the name and description properties, but the diceCoefficient only takes into account the dataset name for sorting.
Including the description in the diceCoefficient will probably slow it down further, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I noticed the description is not even displayed in the advanced dataset view, so it's probably alright to remove "description" in line 66.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Searching for some word of the description doesn't work for me anyways, neither in the advanced nor on the spotlight dataset view, not sure why.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Searching for some word of the description doesn't work for me anyways, neither in the advanced nor on the spotlight dataset view, not sure why.

Really? It works for me 🤔

I'm not sure whether this will result in strange results if the user searches for a word in the description. The search results are filtered using the name and description properties, but the diceCoefficient only takes into account the dataset name for sorting.

My assumption is that the search is only used for dataset name matching, anyway. In the rare case, that a user searches the description, the ordering might be suboptimal (or "random"?), but I don't think that's a showstopper, as the user's had to scroll through a long list before anyway. We can remove the description from the search parameters, but I'd leave it as is for now until someone complains. That way, we don't remove existing functionality from the view.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed that the string Original data and segmentation is the description and I could search for parts of that, but that string is only a default string that is shown if there is no description. Setting a custom description and searching for it works as intended, all fine :)
Let's leave the description in there, your argumentation makes sense to me!

}))
.sortBy("diceCoefficient")
.map(({ row }) => row)
.reverse()
.value()
: filteredDataSource;

return (
<div className="TestAdvancedDatasetView">
<Table
dataSource={Utils.filterWithSearchQueryOR(
this.props.datasets,
["name", "description"],
this.props.searchQuery,
)}
dataSource={sortedDataSource}
rowKey="name"
pagination={{
defaultPageSize: 50,
}}
expandedRowRender={dataset => <DatasetAccessListView dataset={dataset} />}
onChange={this.handleChange}
>
<Column
title="Name"
dataIndex="name"
key="name"
sorter={Utils.localeCompareBy(typeHint, "name")}
sortOrder={sortedInfo.columnKey === "name" && sortedInfo.order}
render={(name: string, dataset: DatasetType) => (
<div>
{dataset.name}
Expand All @@ -55,8 +115,8 @@ class AdvancedDatasetView extends React.PureComponent<Props> {
title="Creation Date"
dataIndex="created"
key="created"
defaultSortOrder="descend"
sorter={Utils.localeCompareBy(typeHint, "formattedCreated")}
sortOrder={sortedInfo.columnKey === "created" && sortedInfo.order}
render={(__, dataset: DatasetType) => dataset.formattedCreated}
/>
<Column
Expand Down Expand Up @@ -91,6 +151,7 @@ class AdvancedDatasetView extends React.PureComponent<Props> {
key="isActive"
width={100}
sorter={(a, b) => a.isActive - b.isActive}
sortOrder={sortedInfo.columnKey === "isActive" && sortedInfo.order}
render={(isActive: boolean) => {
const icon = isActive ? "check-circle-o" : "close-circle-o";
return <Icon type={icon} style={{ fontSize: 20 }} />;
Expand All @@ -102,6 +163,7 @@ class AdvancedDatasetView extends React.PureComponent<Props> {
key="isPublic"
width={100}
sorter={(a, b) => a.isPublic - b.isPublic}
sortOrder={sortedInfo.columnKey === "isPublic" && sortedInfo.order}
render={(isPublic: boolean) => {
const icon = isPublic ? "check-circle-o" : "close-circle-o";
return <Icon type={icon} style={{ fontSize: 20 }} />;
Expand Down
Loading