-
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
Conversation
Was there a reason that you didn't use the Tree component? |
d17a50a
to
0909d8c
Compare
0909d8c
to
82c3508
Compare
Updated to master, now uses Tree component. |
82c3508
to
979def2
Compare
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.
Thanks, @washort. This is definitely moving in the right direction.
Some things I noticed in this version:
- Search doesn't seem to work properly: it's case sensitive and doesn't search in table names.
- The "insert name into query text" arrow should appear only on hover and should be right aligned.
- The padding is too large between items and the columns are not aligned with the table name.
- When clicking on a table name nothing happens (it should expand the table and show columns).
- No table icon.
Also, did you test it with large schemas?
979def2
to
a61acdc
Compare
a61acdc
to
7d78e2f
Compare
e8f6b37
to
7b0a256
Compare
This is ready for a re-review. Please let me know if anything else needs to be addressed. |
While you addressed the issues from my previous comment, you have introduced new issues:
|
I don't see any of that locally though I am trying to repair my local environment since the worker processes kept erroring out after the rebase. It should be almost identical to the angular version granted the debounce can be removed/lowered and it also preserves the flex overflow ellipsis. Appreciate the quick reply though I'll check if ashort and others can see the same issue in the branch before asking for rereview. |
@spasovski This is a screenshot from this PR's Netlify preview: Comparing to |
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.
@spasovski I think AntD's Menu and SubMenu would be a better fit than Tree.
@@ -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 comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Yep agreed.
Lol I didn't realize you were asked to switch to Tree 😵 |
@spasovski which Ant component do you think would be best here? |
@ranbena I think the best is to keep existing markup - it also doesn't require any CSS changes. In this case it's really simple component. |
7b0a256
to
b137b3d
Compare
830e146
to
7df7f18
Compare
This is ready for another review pass. Uses antd's |
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.
Thanks for the update. Two issues I noticed:
1.
Switch to a data source that doesn't have schema (like "Gists" on the preview instance) and then back to one with schema. Result: it no longer shows the schema.
2.
When searching, it seems to reset to showing everything for a brief moment and then show the filtered results. Creates confusing flickering.
Also, did you test it with a really large schema (thousands of columns/tables)?
); | ||
|
||
doRowToggle = ({ target }) => { | ||
target.closest('.table-item').classList.toggle('schema-table-expanded'); |
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.
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 - isExpanded
. Also, it's a good case for using React hooks (useState
).
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.
BTW - with this approach you can even remove css class, and just conditionally render columns only when table is expanded.
0ab8c0d
to
5206f62
Compare
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.
Some new notices. Also, please don't ignore suggestions from previous reviews.
.table-open { | ||
display: none; | ||
|
||
.copyArrow { |
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
|
||
import { Schema } from '../proptypes'; | ||
|
||
const isSchemaEmpty = schema => schema === undefined || !schema.length; |
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.
isArray(schema) && (schema.length > 0)
:
- it's self-explaining: we expect that schema is an array with non-zero length.
- it's more reliable (what if schema will be null? object?)
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.
what about:
const isSchemaValid = schema => schema.constructor === Array && schema.length;
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.
isArray
=== schema.constructor === Array
, but it's shorter and literally means what we need to check - that argument "is array" (vs. "argument's constructor is equal to Array") - that's rather a question of semantics.
componentDidUpdate(prevProps, prevState) { | ||
if (prevProps.schema && this.props.schema.length && (prevState.schema.length !== this.props.schema.length)) { | ||
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Use this with a list of rules you want to disable
if (prevProps.schema && this.props.schema.length && (prevState.schema.length !== this.props.schema.length)) { | ||
// setState() is fine here within the right conditions. | ||
// eslint-disable-next-line | ||
if (!this.state.schemaInitialized) this.setState({ schema: this.props.schema }); |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
evt.target.value
is always string; here you should just update state and call doSearch
, which should properly handle empty search term (return entire schema).
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Seems you don't need handleSearch
at all. This function should look line this:
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: onChange={event => this.doSearch(event.target.value)}
.
Also, move debounce call here as well.
return ( | ||
<div className="schema-container"> | ||
<div className="schema-control"> | ||
<input |
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.
Use Input
and Button
components from Ant
} | ||
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
On query page, $scope.schema
is set to a new array each time it's updated. So this check could be simplified to prevProps.schema !== this.props.schema
. Also, it's the only condition needed - component should be ready that this prop may change - in this case filter should be applied to new schema.
The PR is not ready for re-review - I should've realized you'd get pinged. I can close until it's ready for next round if you'd prefer. |
// 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 comment
The 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 g
modifier makes regex "sticky" - it will remember last position where match was found, and next call to test
will start matching from that position.
Second attempt. init.init was missing in the new component. This will look different due to Antd though I can make it more closely resemble the current UI if requested.