-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
WIP: implement schema browser as a react component #3140
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 |
---|---|---|
|
@@ -69,12 +69,12 @@ function createRowRenderTemplate(columns, $compile) { | |
switch (column.displayAs) { | ||
case 'json': | ||
return ` | ||
<dynamic-table-json-cell column="columns[${index}]" | ||
<dynamic-table-json-cell column="columns[${index}]" | ||
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 it somehow related to this PR? 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. An earlier linter was whining about this though it is otherwise unrelated. Will gladly remove the whitespace change if needed. 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. Interesting. What errors did you get from linter? 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. ping 🙂 I'm really wondering why there are any codestyle warnings for this code - because that's string literal and all linters shouldn't touch it. 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. I am probably not remembering right. I think (in november of last year) there was something the linter required here (possibly unused import or something) and after I made the change my code editor removed the trailing whitespace. I will go into vi and re-add it. 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. Which editor do you use? Because my one also removes trailing whitespaces, but keeps in multiline string literals (because in general case it's unsafe to remove them) 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. And while it's not so important, but I think you should revert this - because the entire file is not related to changes declared in this PR. 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. Yep agreed. |
||
value="row[columns[${index}].name]"></dynamic-table-json-cell> | ||
`; | ||
default: | ||
return ` | ||
<dynamic-table-default-cell column="columns[${index}]" | ||
<dynamic-table-default-cell column="columns[${index}]" | ||
row="row"></dynamic-table-default-cell> | ||
`; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,135 @@ | ||
import React from 'react'; | ||
import { react2angular } from 'react2angular'; | ||
import PropTypes from 'prop-types'; | ||
import { Icon } from 'antd'; | ||
import { debounce } from 'lodash'; | ||
import { $rootScope } from '@/services/ng'; | ||
|
||
import { Schema } from '../proptypes'; | ||
|
||
const isSchemaEmpty = schema => schema === undefined || !schema.length; | ||
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.
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. what about: 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.
|
||
|
||
// Return true if the search term is contained within the table name | ||
// or any of the table's columns. | ||
const doesTableContainSearchTerm = (searchTerm, table) => { | ||
const regEx = new RegExp(searchTerm, 'gi'); | ||
|
||
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. One more notice about this function: avoid using regex, especially in this form. First of all, it allows users to use arbitrary regex to filter schema, as well as it breaks previous behavior. Also, using |
||
// If the table name matches the search term we can return early. | ||
if (regEx.test(table.name)) return true; | ||
|
||
return !!table.columns.filter(column => regEx.test(column)).length; | ||
}; | ||
|
||
const doCopy = (evt, hierarchy) => { | ||
// eslint-disable-next-line | ||
$rootScope.$broadcast('query-editor.command', 'paste', hierarchy.join('.')); | ||
evt.stopPropagation(); | ||
}; | ||
|
||
class SchemaBrowser extends React.Component { | ||
static propTypes = { | ||
schema: Schema, // eslint-disable-line react/no-unused-prop-types | ||
doRefresh: PropTypes.func.isRequired, | ||
}; | ||
|
||
static defaultProps = { | ||
schema: null, | ||
}; | ||
|
||
constructor(props) { | ||
super(props); | ||
|
||
this.state = { | ||
searchTerm: '', | ||
schema: [], | ||
schemaInitialized: false, | ||
}; | ||
|
||
// Candidate to be abstracted out to a set of config constants. | ||
// Performs search filtering after the number of milliseconds set below. | ||
this.doSearch = debounce(this.doSearch, 150); | ||
} | ||
|
||
componentDidUpdate(prevProps, prevState) { | ||
if (prevProps.schema && (this.props.schema && prevState.schema.length !== this.props.schema.length)) { | ||
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. On query page, |
||
// setState() is fine here within the right conditions. | ||
// eslint-disable-next-line | ||
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. Use this with a list of rules you want to disable |
||
if (!this.state.schemaInitialized) this.setState({ schema: this.props.schema }); | ||
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. This is the line where eslint should fail (we require block even for a single statement). |
||
} | ||
} | ||
|
||
handleSearch = (evt) => { | ||
if (!evt.target.value) { | ||
this.setState({ schema: this.props.schema, searchTerm: '' }); | ||
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.
|
||
return; | ||
} | ||
this.setState({ searchTerm: evt.target.value }); | ||
this.doSearch(evt.target.value); | ||
} | ||
|
||
doSearch = (searchTerm) => { | ||
const schema = this.props.schema.filter(table => doesTableContainSearchTerm(searchTerm, table)); | ||
this.setState({ searchTerm, schema }); | ||
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. Seems you don't need doSearch = (searchTerm) => {
let { schema } = this.props;
if (searchTerm !== '') {
schema = filter(schema, table => doesTableContainSearchTerm(searchTerm, table));
}
this.setState({ searchTerm, schema });
} Then use it directly with input: Also, move debounce call here as well. |
||
} | ||
|
||
getTitleElement = (title, isTableRow) => ( | ||
<span className="schema-browser-title-item"> | ||
{isTableRow && <Icon type="table" />} | ||
<span>{` ${title} `}</span> | ||
<Icon | ||
onClick={(evt) => { doCopy(evt, [title]); }} | ||
aria-hidden="true" | ||
type="double-right" | ||
className="copy-arrow" | ||
/> | ||
</span> | ||
); | ||
|
||
doRowToggle = ({ target }) => { | ||
target.closest('.table-item').classList.toggle('schema-table-expanded'); | ||
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. I think it should be implemented in more "native" way: create a component that will represent a single table. That component will accept table name and columns via props (also add other necessary props), and will have the only state field - 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. BTW - with this approach you can even remove css class, and just conditionally render columns only when table is expanded. |
||
} | ||
|
||
render() { | ||
if (!this.props.schema) { | ||
return null; | ||
} | ||
return ( | ||
<div className="schema-container"> | ||
<div className="schema-control"> | ||
<input | ||
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. Use |
||
type="text" | ||
placeholder="Search schema..." | ||
className="form-control" | ||
disabled={isSchemaEmpty(this.props.schema)} | ||
value={this.state.searchTerm} | ||
onChange={this.handleSearch} | ||
/> | ||
<button | ||
className="btn btn-default" | ||
title="Refresh Schema" | ||
type="button" | ||
onClick={this.props.doRefresh} | ||
> | ||
<span className="zmdi zmdi-refresh" /> | ||
</button> | ||
</div> | ||
<div className="schema-browser"> | ||
{this.state.schema.map(table => ( | ||
<div key={table.name} className="table-item"> | ||
<div className="table-name" onClick={this.doRowToggle}>{this.getTitleElement(table.name, true)}</div> | ||
{table.columns.map(column => ( | ||
<div className="table-open" key={column}>{this.getTitleElement(column)}</div> | ||
))} | ||
</div> | ||
))} | ||
</div> | ||
</div> | ||
); | ||
} | ||
} | ||
|
||
export default function init(ngModule) { | ||
ngModule.component('schemaBrowser', react2angular(SchemaBrowser)); | ||
} | ||
|
||
init.init = true; |
This file was deleted.
This file was deleted.
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.
kebab-case