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

Added <QuerySelector /> component #3494

Merged
merged 5 commits into from
Feb 28, 2019
Merged

Added <QuerySelector /> component #3494

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a50cab
Updated npm to support react hooks
ranbena Feb 26, 2019
0d209cb
Added <QuerySelector />
ranbena Feb 25, 2019
cd9f1e0
Changed selectQuery to also clear, completed 2->3 dots in msg, avoidi…
ranbena Feb 27, 2019
325be9f
Removed unused highlight lib
ranbena Feb 27, 2019
a5b0aaf
Merge branch 'master' into query-selector
ranbena Feb 28, 2019
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
1 change: 1 addition & 0 deletions client/.eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ module.exports = {
'no-param-reassign': 0,
'no-mixed-operators': 0,
'no-underscore-dangle': 0,
"no-use-before-define": ["error", "nofunc"],
ranbena marked this conversation as resolved.
Show resolved Hide resolved
"prefer-destructuring": "off",
"prefer-template": "off",
"no-restricted-properties": "off",
Expand Down
160 changes: 160 additions & 0 deletions client/app/components/QuerySelector.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
import React, { useState, useEffect } from 'react';
import PropTypes from 'prop-types';
import cx from 'classnames';
import { debounce, find } from 'lodash';
import Input from 'antd/lib/input';
import { Query } from '@/services/query';
import { toastr } from '@/services/ng';
import { QueryTagsControl } from '@/components/tags-control/TagsControl';

const SEARCH_DEBOUNCE_DURATION = 200;

class StaleSearchError extends Error {
constructor() {
super('stale search');
}
}

function search(term) {
// get recent
if (!term) {
return Query.recent().$promise
.then((results) => {
const filteredResults = results.filter(item => !item.is_draft); // filter out draft
return Promise.resolve(filteredResults);
});
}

// search by query
return Query.query({ q: term }).$promise
.then(({ results }) => Promise.resolve(results));
}

export function QuerySelector(props) {
const [searchTerm, setSearchTerm] = useState();
const [searching, setSearching] = useState();
const [searchResults, setSearchResults] = useState([]);
const [selectedQuery, setSelectedQuery] = useState();

let isStaleSearch = false;
const debouncedSearch = debounce(_search, SEARCH_DEBOUNCE_DURATION);
const placeholder = 'Search a query by name';
const clearIcon = <i className="fa fa-times" onClick={() => selectQuery(null)} />;
const spinIcon = <i className={cx('fa fa-spinner fa-pulse', { hidden: !searching })} />;

// set selected from prop
useEffect(() => {
if (props.selectedQuery) {
setSelectedQuery(props.selectedQuery);
}
arikfr marked this conversation as resolved.
Show resolved Hide resolved
}, [props.selectedQuery]);

// on search term changed, debounced
useEffect(() => {
// clear results, no search
if (searchTerm === null) {
setSearchResults(null);
return () => {};
}

// search
debouncedSearch(searchTerm);
return () => {
debouncedSearch.cancel();
isStaleSearch = true;
};
}, [searchTerm]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ranbena Look: you still load search results as a side-effect of changing search term. It's implicit dependency, that's what makes code hard to read and understand:

$scope.setSearchTerm = (searchTerm) => {
  $scope.searchTerm = searchTerm;
};

// somewhere else

$scope.$watch('searchTerm', (searchTerm) => {
  if (searchTerm == '') {
    $scope.results = [];
  } else {
    doSearch(searchTerm);
  }
});

in opposite to explicit reaction, where you immediately see that if you change search term - it will do some extra stuff:

$scope.setSearchTerm = (searchTerm) => {
  $scope.searchTerm = searchTerm;
  if (searchTerm == '') {
    $scope.results = [];
  } else {
    doSearch(searchTerm);
  }
};

I intentionally provide examples in Angular because this pattern is really common there, and one of best practices in React for a long time was to avoid such code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this useEffect also allows easy stale/debounce management (the alternative I assume is having dedicated refs), I'd rather keep it and keep moving 🏃


function _search(term) {
setSearching(true);
search(term)
.then(rejectStale)
.then((results) => {
setSearchResults(results);
setSearching(false);
arikfr marked this conversation as resolved.
Show resolved Hide resolved
})
.catch((err) => {
if (!(err instanceof StaleSearchError)) {
setSearching(false);
}
});
}

function rejectStale(results) {
return isStaleSearch
? Promise.reject(new StaleSearchError())
: Promise.resolve(results);
}

function selectQuery(queryId) {
let query = null;
if (queryId) {
query = find(searchResults, { id: queryId });
if (!query) { // shouldn't happen
toastr.error('Something went wrong... Couldn\'t select query');
}
}

setSearchTerm(query ? null : ''); // empty string triggers recent fetch
Copy link
Collaborator

Choose a reason for hiding this comment

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

It took some time for me to understand how it works, and then I realized that actually it's AngularJS-way to handle data flow: you set a state field, but setter just updates value; and there is watcher ($scope.$watch) that does something when that field changes. That's not something I personally expect to see in React code 🤔

Copy link
Contributor Author

@ranbena ranbena Feb 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I do it rarely when it makes sense. But now considering more since useEffect is so convenient. Will stay away from it if you feel it's an anti-pattern.

Copy link
Collaborator

@kravets-levko kravets-levko Feb 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Will stay away from it if you feel it's an anti-pattern.

Yes, I think so. That's what makes AngularJS code hard to understand; it's much easier to understand what's going on when you see: "ah, this function updates state variable, but also it loads queries", etc. With such side effects you see: "this function updates state. okay." - and then you see that oh magic! it somehow loads data! whaaat?? And you need to find another place in the code which load query results. And BTW - in AngularJS this mechanism is a bit better because you describe dependency first, and then write handler. With useEffect if's really easy to miss dependencies (what I did, actually). Compare:

$scope.$watch('field', () => { // <- no chance to miss it
  ...
  // a lot of code
  ...
});

useEffect(() => {
  ...
  // a lot of code
  ...
}, [field]); // <- this line left unattended

I really think this is anti-pattern; probably one of the things I most liked in React - strict data flows, without implicit dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting observation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Read that code once more and have something to add)

Another con of this approach is harder data flow control. Did you notice that effect that loads search results could be optimized? When search term is null - you already have search results (empty list). If you have setter for search term - you can handle it immediately:

  1. user clicks "clear" button - search term set to null, and search results to null;
  2. component renders because state changed.

but with useEffect it will:

  1. user clicks "clear" button - search term set to null;
  2. component renders because state changed;
  3. side effect executed, it sees that search term is null and updates search results to null
  4. component renders because state changed.

setSelectedQuery(query);
props.onChange(query);
}

function renderResults() {
if (!searchResults.length) {
return <div className="text-muted">No results matching search term.</div>;
}

return (
<div className="list-group">
{searchResults.map(q => (
<a
href="javascript:void(0)"
className={cx('list-group-item', { inactive: q.is_draft })}
key={q.id}
onClick={() => selectQuery(q.id)}
>
{q.name}
kravets-levko marked this conversation as resolved.
Show resolved Hide resolved
{' '}
<QueryTagsControl isDraft={q.is_draft} tags={q.tags} className="inline-tags-control" />
</a>
))}
</div>
);
}

if (props.disabled) {
return <Input value={selectedQuery && selectedQuery.name} placeholder={placeholder} disabled />;
}

return (
<React.Fragment>
{selectedQuery ? (
<Input value={selectedQuery.name} suffix={clearIcon} readOnly />
) : (
<Input
placeholder={placeholder}
value={searchTerm}
onChange={e => setSearchTerm(e.target.value)}
suffix={spinIcon}
/>
)}
<div className="scrollbox" style={{ maxHeight: '50vh', marginTop: 15 }}>
ranbena marked this conversation as resolved.
Show resolved Hide resolved
{searchResults && renderResults()}
</div>
</React.Fragment>
);
}

QuerySelector.propTypes = {
onChange: PropTypes.func.isRequired,
selectedQuery: PropTypes.object, // eslint-disable-line react/forbid-prop-types
disabled: PropTypes.bool,
};

QuerySelector.defaultProps = {
selectedQuery: null,
disabled: false,
};

export default QuerySelector;
162 changes: 12 additions & 150 deletions client/app/components/dashboards/AddWidgetDialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,16 @@
import { debounce, each, values, map, includes, first, identity } from 'lodash';
import { each, values, map, includes, first } from 'lodash';
import React from 'react';
import PropTypes from 'prop-types';
import Select from 'antd/lib/select';
import Modal from 'antd/lib/modal';
import { wrap as wrapDialog, DialogPropType } from '@/components/DialogWrapper';
import { BigMessage } from '@/components/BigMessage';
import highlight from '@/lib/highlight';
import {
MappingType,
ParameterMappingListInput,
editableMappingsToParameterMappings,
synchronizeWidgetTitles,
} from '@/components/ParameterMappingInput';
import { QueryTagsControl } from '@/components/tags-control/TagsControl';
import { QuerySelector } from '@/components/QuerySelector';

import { toastr } from '@/services/ng';
import { Widget } from '@/services/widget';
Expand All @@ -26,51 +24,23 @@ class AddWidgetDialog extends React.Component {
dialog: DialogPropType.isRequired,
};

constructor(props) {
super(props);
this.state = {
saveInProgress: false,
selectedQuery: null,
searchTerm: '',
highlightSearchTerm: false,
recentQueries: [],
queries: [],
selectedVis: null,
parameterMappings: [],
isLoaded: false,
};

const searchQueries = debounce(this.searchQueries.bind(this), 200);
this.onSearchTermChanged = (event) => {
const searchTerm = event.target.value;
this.setState({ searchTerm });
searchQueries(searchTerm);
};
}

componentDidMount() {
Query.recent().$promise.then((items) => {
// Don't show draft (unpublished) queries in recent queries.
const results = items.filter(item => !item.is_draft);
this.setState({
recentQueries: results,
queries: results,
isLoaded: true,
highlightSearchTerm: false,
});
});
}
state = {
saveInProgress: false,
selectedQuery: null,
selectedVis: null,
parameterMappings: [],
};

selectQuery(queryId) {
selectQuery(selectedQuery) {
// Clear previously selected query (if any)
this.setState({
selectedQuery: null,
selectedVis: null,
parameterMappings: [],
});

if (queryId) {
Query.get({ id: queryId }, (query) => {
if (selectedQuery) {
Query.get({ id: selectedQuery.id }, (query) => {
if (query) {
const existingParamNames = map(
this.props.dashboard.getParametersDefs(),
Expand All @@ -96,31 +66,6 @@ class AddWidgetDialog extends React.Component {
}
}

searchQueries(term) {
if (!term || term.length === 0) {
this.setState(prevState => ({
queries: prevState.recentQueries,
isLoaded: true,
highlightSearchTerm: false,
}));
return;
}

Query.query({ q: term }, (results) => {
// If user will type too quick - it's possible that there will be
// several requests running simultaneously. So we need to check
// which results are matching current search term and ignore
// outdated results.
if (this.state.searchTerm === term) {
this.setState({
queries: results.results,
isLoaded: true,
highlightSearchTerm: true,
});
}
});
}

selectVisualization(query, visualizationId) {
each(query.visualizations, (visualization) => {
if (visualization.id === visualizationId) {
Expand Down Expand Up @@ -173,88 +118,6 @@ class AddWidgetDialog extends React.Component {
this.setState({ parameterMappings });
}

renderQueryInput() {
return (
<div className="form-group">
{!this.state.selectedQuery && (
<input
type="text"
placeholder="Search a query by name"
className="form-control"
value={this.state.searchTerm}
onChange={this.onSearchTermChanged}
/>
)}
{this.state.selectedQuery && (
<div className="p-relative">
<input type="text" className="form-control bg-white" value={this.state.selectedQuery.name} readOnly />
<a
href="javascript:void(0)"
onClick={() => this.selectQuery(null)}
className="d-flex align-items-center justify-content-center"
style={{
position: 'absolute',
right: '1px',
top: '1px',
bottom: '1px',
width: '30px',
background: '#fff',
borderRadius: '3px',
}}
>
<i className="text-muted fa fa-times" />
</a>
</div>
)}
</div>
);
}

renderSearchQueryResults() {
const { isLoaded, queries, highlightSearchTerm, searchTerm } = this.state;

const highlightSearchResult = highlightSearchTerm ? highlight : identity;

return (
<div className="scrollbox" style={{ maxHeight: '50vh' }}>
{!isLoaded && (
<div className="text-center">
<BigMessage icon="fa-spinner fa-2x fa-pulse" message="Loading..." />
</div>
)}

{isLoaded && (
<div>
{
(queries.length === 0) &&
<div className="text-muted">No results matching search term.</div>
}
{(queries.length > 0) && (
<div className="list-group">
{queries.map(query => (
<a
href="javascript:void(0)"
className={'list-group-item ' + (query.is_draft ? 'inactive' : '')}
key={query.id}
onClick={() => this.selectQuery(query.id)}
>
<div
// eslint-disable-next-line react/no-danger
dangerouslySetInnerHTML={{ __html: highlightSearchResult(query.name, searchTerm) }}
style={{ display: 'inline-block' }}
/>
{' '}
<QueryTagsControl isDraft={query.is_draft} tags={query.tags} className="inline-tags-control" />
</a>
))}
</div>
)}
</div>
)}
</div>
);
}

renderVisualizationInput() {
let visualizationGroups = {};
if (this.state.selectedQuery) {
Expand Down Expand Up @@ -304,8 +167,7 @@ class AddWidgetDialog extends React.Component {
okText="Add to Dashboard"
width={700}
>
{this.renderQueryInput()}
{!this.state.selectedQuery && this.renderSearchQueryResults()}
<QuerySelector onChange={query => this.selectQuery(query)} />
{this.state.selectedQuery && this.renderVisualizationInput()}

{
Expand Down
Loading