-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature new query builder #22
Conversation
…_new_query_builder
…_new_query_builder
@@ -28,11 +28,11 @@ define(['jquery'], function () { | |||
THREE.FilmPass = require('imports-loader?THREE=three!exports-loader?THREE.FilmPass!three\/examples\/js\/postprocessing\/FilmPass'); |
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.
@ddelpiano why is this file changed here in this branch? I see mostly formatting but also some actual changes and I thought this is stuff that was a untouched since we solved those other issues months ago
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.
there is a single line changed, the canvas was detecting the click when the querybuilder is open but the coordinates were undefined since the action was not performed on the canvas itself, I had to add that check to avoid this issue.
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.
Ah ok got it, I forgot and I thought unrelated to query stuff. In particular if there is a singel change trying to not do a lot of formatting at the same time otherwise it's very hard to spot what actually changed 👍
var actionStr = that.props.metadata.actions; | ||
actionStr = actionStr.replace(/\$entity\$/gi, path); | ||
GEPPETTO.CommandController.execute(actionStr); | ||
GEPPETTO.QueryBuilder.close(); |
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.
@ddelpiano commandController is ok but would be good to remove the GEPPETTO.QueryBuilder reference as part of this refactoring
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.
|
||
render () { | ||
// TODO: would be nicer to pass controls and config straight from the parent component rather than assume | ||
var config = GEPPETTO.QueryBuilder.state.resultsControlsConfig; |
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.
@ddelpiano same, we should remove GEPPETTO.QueryBuilder referencing from within these components ideally
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.
as per my reply above.
|
||
showBrentSpiner: function (spin) { | ||
showBrentSpiner (spin) { |
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.
@ddelpiano anybody catch this star trek reference yet? 😂
useGriddleStyles={false} columnMetadata={this.state.resultsColumnMeta} /> | ||
</Typography> | ||
return (focusTabIndex === index | ||
&& <Typography component="div" key={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.
@ddelpiano something upsets me in this indentation 😬
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.
already discussed, blame the linter or @rodriguez-facundo in case 😂
…e part of the componentWillMount of the queryBuilder itself, passing all the configs as props. Also the global GEPPETTO.Querybuilder as been removed almost everywhere
@gidili this is ready to be reviewed again, thx |
Important notes:
Note, the columns name have to respect the convention where the field columnName is the lowercase version of the headers that arrive from the query processor defined in the backend, if these names don't match the filter will not work so the developers need to know in advance the columns name that will arrive from the backend.
Note, as discussed previously, even if a column is not defined in this array will be placed inside the csv that the use can download, but not displayed.
(e.g. GEPPETTO.QueryBuilder.close(); ).