WIP: implement schema browser as a react component #3140

spasovski · 2018-11-28T19:50:29Z

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.

arikfr · 2018-11-28T20:34:36Z

Was there a reason that you didn't use the Tree component?

washort · 2019-01-16T04:37:04Z

Updated to master, now uses Tree component.

arikfr

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?

spasovski · 2019-03-05T17:08:53Z

This is ready for a re-review. Please let me know if anything else needs to be addressed.

arikfr · 2019-03-05T19:14:39Z

While you addressed the issues from my previous comment, you have introduced new issues:

Why the columns on the right?
When clicking on a column it gets blue background color. There is not supposed to be a click behavior for columns.
Search is very unresponsive -- maybe because of the 500ms debounce?
The search query seems to repeat key strokes: type some word like Album then delete it with the backspace, why a moment you will see A returns into the search query.

spasovski · 2019-03-05T19:25:18Z

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.

kravets-levko · 2019-03-05T19:33:24Z

@spasovski This is a screenshot from this PR's Netlify preview:

Comparing to master, it also misses some margins around schema browser:

ranbena

@spasovski I think AntD's Menu and SubMenu would be a better fit than Tree.

kravets-levko · 2019-03-05T19:35:44Z

client/app/components/dynamic-table/index.js

@@ -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}]"


Is it somehow related to this PR?

An earlier linter was whining about this though it is otherwise unrelated. Will gladly remove the whitespace change if needed.

Interesting. What errors did you get from linter?

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.

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.

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)

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.

Yep agreed.

ranbena · 2019-03-05T19:38:03Z

@spasovski I think AntD's Menu and SubMenu would be a better fit than Tree.

Lol I didn't realize you were asked to switch to Tree 😵
Lemme get back to you on that.

client/app/components/queries/schema-browser.html

ranbena · 2019-03-05T19:53:37Z

@spasovski which Ant component do you think would be best here?

kravets-levko · 2019-03-05T19:55:33Z

@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.

spasovski · 2019-03-13T00:53:19Z

This is ready for another review pass. Uses antd's <Icon> but is regular markup otherwise.

arikfr

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)?

client/app/assets/less/ant.less

kravets-levko · 2019-03-13T09:27:27Z

client/app/components/queries/SchemaBrowser.jsx

+  );
+
+  doRowToggle = ({ target }) => {
+    target.closest('.table-item').classList.toggle('schema-table-expanded');


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).

BTW - with this approach you can even remove css class, and just conditionally render columns only when table is expanded.

client/app/components/queries/SchemaBrowser.jsx

kravets-levko

Some new notices. Also, please don't ignore suggestions from previous reviews.

kravets-levko · 2019-03-19T18:23:45Z

client/app/assets/less/ant.less

+  .table-open {
+    display: none;
+
+    .copyArrow {


kravets-levko · 2019-03-19T18:25:46Z

client/app/components/queries/SchemaBrowser.jsx

+
+import { Schema } from '../proptypes';
+
+const isSchemaEmpty = schema => schema === undefined || !schema.length;


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?)

what about:
const isSchemaValid = schema => schema.constructor === Array && schema.length;

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.

kravets-levko · 2019-03-19T18:27:57Z

client/app/components/queries/SchemaBrowser.jsx

+  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


Use this with a list of rules you want to disable

kravets-levko · 2019-03-19T18:28:50Z

client/app/components/queries/SchemaBrowser.jsx

+    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 });


This is the line where eslint should fail (we require block even for a single statement).

kravets-levko · 2019-03-19T18:30:53Z

client/app/components/queries/SchemaBrowser.jsx

+
+  handleSearch = (evt) => {
+    if (!evt.target.value) {
+      this.setState({ schema: this.props.schema, searchTerm: '' });


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).

kravets-levko · 2019-03-19T18:35:27Z

client/app/components/queries/SchemaBrowser.jsx

+
+  doSearch = (searchTerm) => {
+    const schema = this.props.schema.filter(table => doesTableContainSearchTerm(searchTerm, table));
+    this.setState({ searchTerm, schema });


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.

kravets-levko · 2019-03-19T18:35:56Z

client/app/components/queries/SchemaBrowser.jsx

+    return (
+      <div className="schema-container">
+        <div className="schema-control">
+          <input


Use Input and Button components from Ant

kravets-levko · 2019-03-19T18:41:23Z

client/app/components/queries/SchemaBrowser.jsx

+  }
+
+  componentDidUpdate(prevProps, prevState) {
+    if (prevProps.schema && (this.props.schema && prevState.schema.length !== this.props.schema.length)) {


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.

spasovski · 2019-03-19T18:54:49Z

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.

kravets-levko · 2019-03-19T19:13:50Z

client/app/components/queries/SchemaBrowser.jsx

+// or any of the table's columns.
+const doesTableContainSearchTerm = (searchTerm, table) => {
+  const regEx = new RegExp(searchTerm, 'gi');
+


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.

arikfr changed the title ~~implement schema browser as a react component~~ WIP: implement schema browser as a react component Jan 3, 2019

washort force-pushed the react-schema-browser branch from d17a50a to 0909d8c Compare January 16, 2019 04:33

ghost assigned washort Jan 16, 2019

ghost added in progress labels Jan 16, 2019

washort force-pushed the react-schema-browser branch from 0909d8c to 82c3508 Compare January 16, 2019 04:35

washort force-pushed the react-schema-browser branch from 82c3508 to 979def2 Compare January 16, 2019 05:45

arikfr requested changes Jan 16, 2019

View reviewed changes

arikfr added Frontend Frontend: React Frontend codebase migration to React labels Jan 16, 2019

washort force-pushed the react-schema-browser branch from 979def2 to a61acdc Compare January 16, 2019 17:48

arikfr added PR: reviewed-changes-requested and removed review labels Jan 23, 2019

washort force-pushed the react-schema-browser branch from a61acdc to 7d78e2f Compare February 15, 2019 19:10

spasovski force-pushed the react-schema-browser branch 3 times, most recently from e8f6b37 to 7b0a256 Compare March 5, 2019 16:52

kravets-levko requested a review from arikfr March 5, 2019 17:10

ranbena self-requested a review March 5, 2019 19:27

ranbena suggested changes Mar 5, 2019

View reviewed changes

kravets-levko reviewed Mar 5, 2019

View reviewed changes

client/app/components/queries/schema-browser.html Show resolved Hide resolved

kravets-levko self-requested a review March 5, 2019 19:39

arikfr removed the PR: reviewed-changes-requested label Mar 6, 2019

spasovski force-pushed the react-schema-browser branch from 7b0a256 to b137b3d Compare March 7, 2019 03:57

weekly-digest bot mentioned this pull request Mar 10, 2019

Weekly Digest (3 March, 2019 - 10 March, 2019) #3560

Closed

implement schema browser as a react component

31a7273

spasovski force-pushed the react-schema-browser branch 2 times, most recently from 830e146 to 7df7f18 Compare March 12, 2019 23:17

arikfr requested changes Mar 13, 2019

View reviewed changes

kravets-levko requested changes Mar 13, 2019

View reviewed changes

address arik PR review comments

5206f62

spasovski force-pushed the react-schema-browser branch from 0ab8c0d to 5206f62 Compare March 19, 2019 18:32

kravets-levko requested changes Mar 19, 2019

View reviewed changes

kravets-levko reviewed Mar 19, 2019

View reviewed changes

arikfr removed the in progress label Oct 27, 2019

weekly-digest bot mentioned this pull request Oct 28, 2019

Weekly Digest (21 October, 2019 - 28 October, 2019) #4308

Closed

kravets-levko mentioned this pull request Dec 10, 2019

Migrate schema browser to react #4432

Merged

1 task

arikfr closed this Jan 20, 2020


		import { Schema } from '../proptypes';

		const isSchemaEmpty = schema => schema === undefined \|\| !schema.length;

WIP: implement schema browser as a react component #3140

WIP: implement schema browser as a react component #3140

Conversation

spasovski commented Nov 28, 2018

arikfr commented Nov 28, 2018

washort commented Jan 16, 2019

arikfr left a comment • edited Loading

Choose a reason for hiding this comment

spasovski commented Mar 5, 2019

arikfr commented Mar 5, 2019

spasovski commented Mar 5, 2019

kravets-levko commented Mar 5, 2019

ranbena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kravets-levko Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranbena commented Mar 5, 2019

ranbena commented Mar 5, 2019

kravets-levko commented Mar 5, 2019

spasovski commented Mar 13, 2019

arikfr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kravets-levko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kravets-levko Mar 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spasovski commented Mar 19, 2019

Choose a reason for hiding this comment

arikfr left a comment •

edited

Loading

kravets-levko Mar 13, 2019 •

edited

Loading

kravets-levko Mar 19, 2019 •

edited

Loading