-
Notifications
You must be signed in to change notification settings - Fork 28
Add CodeMirror editor with sql syntax and autocomplete #35
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
Conversation
frontend/src/components/QueryBox.js
Outdated
? this.schemaToCodeMirror(this.props.schema) | ||
: {}; | ||
|
||
const options = { |
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.
Isn't really important, but I wouldn't convert schema and create options object on every render (means every key press). Consider to create the codemirror configuration in constructor and update it on componentWillReceiveProps when schema or handleSubmit
are changed.
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.
You are right, fixed in d2fb518
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.
rebased, now it's fixed in ce8f1dd
|
||
.CodeMirror-empty { | ||
color: dimgrey; | ||
} |
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.
new 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.
great!
Code to fix CodeMirror dependencies in frontend tests taken from https://github.com/bblfsh/dashboard/blob/master/src/setupTests.js Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>
.then(res => | ||
Promise.all( | ||
res.data.map(e => | ||
query(`DESCRIBE TABLE ${e.table}`).then(tableRes => ({ |
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.
@carlosms sorry for digging this up, but I have just realized that we need to change this logic eventually.
Instead for
- API call
/tables
- then, for each table call
DESCRIBE TABLE
from the frontend
We should have
- API call to
/tables
, so that the backend returns all this information in a single call
on top of per improvements (less HTTP calls), one of the purposes of having separate API endpoint on the backend was to abstract the details of how meta-information (schema) is collected.
Current approach, for a case if it is not a DESCRIBE TABLE
- would require us to change both, backend and frontend. The one suggested above would requite only the backend change.
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 agree with you comment. I actually though of doing it that way in this PR, but didn't want to get too ahead of myself and went for the quicker approach of doing it in the frontend.
I've created a new issue with your request: #40
Fixes #20.
Besides syntax highlighting this PR also adds autocomplete. To do that, the schema is loaded when the application starts. Also the query can be executed with ctrl+enter.