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

[sqllab] table refactor #2587

Merged
merged 18 commits into from
Apr 18, 2017
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class DataPreviewModal extends React.PureComponent {
</Modal.Title>
</Modal.Header>
<Modal.Body>
<ResultSet query={query} visualize={false} csv={false} actions={this.props.actions} />
<ResultSet query={query} visualize={false} csv={false} actions={this.props.actions} height={400} />
</Modal.Body>
</Modal>
);
Expand Down
4 changes: 3 additions & 1 deletion superset/assets/javascripts/SqlLab/components/QueryTable.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,9 @@ class QueryTable extends React.PureComponent {
modalTitle={'Data preview'}
beforeOpen={this.openAsyncResults.bind(this, query)}
onExit={this.clearQueryResults.bind(this, query)}
modalBody={<ResultSet showSql query={query} actions={this.props.actions} />}
modalBody={
<ResultSet showSql query={query} actions={this.props.actions} height={400} />
}
/>
);
} else {
Expand Down
47 changes: 13 additions & 34 deletions superset/assets/javascripts/SqlLab/components/ResultSet.jsx
Original file line number Diff line number Diff line change
@@ -1,42 +1,40 @@
import React from 'react';
import { Alert, Button, ButtonGroup, ProgressBar } from 'react-bootstrap';
import { Table } from 'reactable';
import shortid from 'shortid';

import VisualizeModal from './VisualizeModal';
import HighlightedSql from './HighlightedSql';

const RESULTS_CONTROLS_HEIGHT = 36;
import FilterableTable from '../../components/FilterableTable/FilterableTable';

const propTypes = {
actions: React.PropTypes.object,
csv: React.PropTypes.bool,
query: React.PropTypes.object,
search: React.PropTypes.bool,
searchText: React.PropTypes.string,
showSql: React.PropTypes.bool,
visualize: React.PropTypes.bool,
cache: React.PropTypes.bool,
resultSetHeight: React.PropTypes.number,
height: React.PropTypes.number.isRequired,
};
const defaultProps = {
search: true,
visualize: true,
showSql: false,
csv: true,
searchText: '',
actions: {},
cache: false,
};

const RESULT_SET_CONTROLS_HEIGHT = 46;

class ResultSet extends React.PureComponent {
export default class ResultSet extends React.PureComponent {
constructor(props) {
super(props);
this.state = {
searchText: '',
showModal: false,
data: [],
height: props.search ? props.height - RESULT_SET_CONTROLS_HEIGHT : props.height,
};
}
componentWillReceiveProps(nextProps) {
Expand All @@ -54,6 +52,7 @@ class ResultSet extends React.PureComponent {
this.fetchResults(nextProps.query);
}
}

getControls() {
if (this.props.search || this.props.visualize || this.props.csv) {
let csvButton;
Expand Down Expand Up @@ -132,6 +131,7 @@ class ResultSet extends React.PureComponent {
reFetchQueryResults(query) {
this.props.actions.reFetchQueryResults(query);
}

render() {
const query = this.props.query;
const results = query.results;
Expand Down Expand Up @@ -195,31 +195,12 @@ class ResultSet extends React.PureComponent {
/>
{this.getControls.bind(this)()}
{sql}
<div
className="ResultSet"
style={{ height: `${this.props.resultSetHeight - RESULTS_CONTROLS_HEIGHT}px` }}
>
<Table
data={data.map(function (row) {
const newRow = {};
for (const k in row) {
const val = row[k];
if (typeof (val) === 'string') {
newRow[k] = val;
} else {
newRow[k] = JSON.stringify(val);
}
}
return newRow;
})}
columns={results.columns.map(col => col.name)}
sortable
className="table table-condensed table-bordered"
filterBy={this.state.searchText}
filterable={results.columns.map(c => c.name)}
hideFilterInput
/>
</div>
<FilterTable
data={data}
columns={results.columns}
height={this.state.height}
searchText={this.state.searchText}
/>
</div>
);
}
Expand All @@ -240,5 +221,3 @@ class ResultSet extends React.PureComponent {
}
ResultSet.propTypes = propTypes;
ResultSet.defaultProps = defaultProps;

export default ResultSet;
4 changes: 2 additions & 2 deletions superset/assets/javascripts/SqlLab/components/SouthPane.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class SouthPane extends React.PureComponent {
search
query={latestQuery}
actions={props.actions}
resultSetHeight={this.state.innerTabHeight}
height={this.state.innerTabHeight}
/>
);
} else {
Expand All @@ -90,7 +90,7 @@ class SouthPane extends React.PureComponent {
csv={false}
actions={props.actions}
cache
resultSetHeight={this.state.innerTabHeight}
height={this.state.innerTabHeight}
/>
</Tab>
));
Expand Down
4 changes: 3 additions & 1 deletion superset/assets/javascripts/SqlLab/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import { initEnhancer } from '../reduxUtils';
import { initJQueryAjaxCSRF } from '../modules/utils';
import App from './components/App';
import { appSetup } from '../common';
import './main.css';

require('./main.css');
require('../components/FilterableTable/FilterableTableStyles.css');

appSetup();
initJQueryAjaxCSRF();
Expand Down
10 changes: 0 additions & 10 deletions superset/assets/javascripts/SqlLab/main.css
Original file line number Diff line number Diff line change
Expand Up @@ -237,16 +237,6 @@ div.tablePopover:hover {
padding-bottom: 3px;
padding-top: 3px;
}
.ResultSet {
overflow: auto;
border-bottom: 1px solid #ccc;
}
.ResultSet table {
margin-bottom: 0px;
}
.ResultSet table tr.last {
border-bottom: none;
}
.ace_editor {
border: 1px solid #ccc;
margin: 0px 0px 10px 0px;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
import { List } from 'immutable';
import React, { PropTypes, PureComponent } from 'react';
import {
Column,
Table,
SortDirection,
SortIndicator,
} from 'react-virtualized';
import { getTextWidth } from '../../modules/visUtils';

const propTypes = {
orderedColumnKeys: PropTypes.array.isRequired,
data: PropTypes.array.isRequired,
height: PropTypes.number.isRequired,
filterText: PropTypes.string,
headerHeight: PropTypes.number,
overscanRowCount: PropTypes.number,
rowHeight: PropTypes.number,
Copy link
Member

Choose a reason for hiding this comment

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

how does this react when it overflow? I saw react-virtualized has a dynamicHeight option too, would that be better?

Copy link
Author

Choose a reason for hiding this comment

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

@mistercrunch and i talked offline about this, we'll keep the static row height and add a max column width. for columns that require truncating the text, we will show a modal or popover... currently there is a tooltip when hovering on any cell which shows the content, but i'm not sure if this is clear enough to the user.

screenshot 2017-04-18 13 57 01

i'll tackle setting a max column width in a follow up PR.

striped: PropTypes.bool,
};

const defaultProps = {
filterText: '',
headerHeight: 32,
overscanRowCount: 10,
rowHeight: 32,
striped: true,
};

export default class FilterableTable extends PureComponent {
constructor(props) {
super(props);
this.list = List(this.formatTableData(props.data));
this.headerRenderer = this.headerRenderer.bind(this);
this.rowClassName = this.rowClassName.bind(this);
this.sort = this.sort.bind(this);

this.widthsForColumnsByKey = this.getWidthsForColumns();
this.totalTableWidth = props.orderedColumnKeys
.map(key => this.widthsForColumnsByKey[key])
.reduce((curr, next) => curr + next);

this.state = {
sortBy: props.orderedColumnKeys[0],
sortDirection: SortDirection.ASC,
fitted: false,
};
}

hasMatch(text, row) {
const values = [];
for (const key in row) {
if (row.hasOwnProperty(key)) {
values.push(row[key].toLowerCase());
}
}
return values.some(v => v.includes(text.toLowerCase()));
}

formatTableData(data) {
const formattedData = data.map((row) => {
const newRow = {};
for (const k in row) {
const val = row[k];
if (typeof(val) === 'string') {
newRow[k] = val;
} else {
newRow[k] = JSON.stringify(val);
}
}
return newRow;
});
return formattedData;
}

getWidthsForColumns() {
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 concerned about the potential cost of this method. getTextWidth creating a canvas object for each cell in the dataset seems costly (though I haven't researched it..). Should be use a sample of rows here?

const PADDING = 40; // accounts for cell padding and width of sorting icon
const widthsByColumnKey = {};
this.props.orderedColumnKeys.forEach((key) => {
const colWidths = this.list.toArray().map(d => getTextWidth(d[key]) + PADDING);
Copy link
Member

Choose a reason for hiding this comment

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

here this.list.toArray() could be called only once outside the for loop, but better would be to stay immutable the whole way through.

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 think you could reduce right off this line already with this.list.max(...)

Copy link
Author

Choose a reason for hiding this comment

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

👍

// push width of column key to array as well
colWidths.push(getTextWidth(key) + PADDING);
// set max width as value for key
widthsByColumnKey[key] = Math.max(...colWidths);
});
return widthsByColumnKey;
}

fitTableToWidthIfNeeded() {
const containerWidth = this.container.getBoundingClientRect().width;
if (containerWidth > this.totalTableWidth) {
this.totalTableWidth = containerWidth - 2; // accomodates 1px border on container
}
this.setState({ fitted: true });
}

componentDidMount() {
this.fitTableToWidthIfNeeded();
}

render() {
const { sortBy, sortDirection } = this.state;
const {
filterText,
headerHeight,
height,
orderedColumnKeys,
overscanRowCount,
rowHeight,
} = this.props;

let sortedAndFilteredList = this.list;
// filter list
if (filterText) {
sortedAndFilteredList = this.list.filter(row => this.hasMatch(filterText, row));
}
// sort list
sortedAndFilteredList = sortedAndFilteredList
.sortBy(item => item[sortBy])
.update(list => sortDirection === SortDirection.DESC ? list.reverse() : list);

const rowGetter = ({ index }) => this.getDatum(sortedAndFilteredList, index);

return (
<div
style={{ height }}
className="filterable-table-container"
ref={(ref) => { this.container = ref; }}
>
{this.state.fitted &&
<Table
ref="Table"
headerHeight={headerHeight}
height={height - 2}
overscanRowCount={overscanRowCount}
rowClassName={this.rowClassName}
rowHeight={rowHeight}
rowGetter={rowGetter}
rowCount={sortedAndFilteredList.size}
sort={this.sort}
sortBy={sortBy}
sortDirection={sortDirection}
width={this.totalTableWidth}
>
{orderedColumnKeys.map((columnKey) => (
<Column
dataKey={columnKey}
disableSort={false}
headerRenderer={this.headerRenderer}
width={this.widthsForColumnsByKey[columnKey]}
label={columnKey}
key={columnKey}
/>
))}
</Table>
}
</div>
);
}

getDatum(list, index) {
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 used to render being the last method at the bottom of the class definition. I think the new linting rules are more picky about method ordering...

return list.get(index % list.size);
}

headerRenderer({ dataKey, label, sortBy, sortDirection }) {
return (
<div>
{label}
{sortBy === dataKey &&
<SortIndicator sortDirection={sortDirection} />
}
</div>
);
}

rowClassName({ index }) {
let className = '';
if (this.props.striped) {
className = index % 2 === 0 ? 'even-row' : 'odd-row';
}
return className;
}

sort({ sortBy, sortDirection }) {
this.setState({ sortBy, sortDirection });
}
}

FilterableTable.propTypes = propTypes;
FilterableTable.defaultProps = defaultProps;
Loading