-
Notifications
You must be signed in to change notification settings - Fork 4
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
Query builder #30
base: master
Are you sure you want to change the base?
Query builder #30
Conversation
- Defined access and refresh tokens - Prepared interceptor for obtaining new access token or redirecting to the login page when refresh token is expired
…page has defined custom variable URL
Available email: [email protected], password: geslo123
…is defined routes are protected by checking if user is logged in and if we have access token defined
…moved into Page component
… (cpu -> 700%). Add * to disable lint.
…moved into src/pql folder
… process moved to pre-push
… can now manage AggregatorOperator
… and added addOrConfigAggregator, build, compileToPql, run, save methods
…ated with new concept
const loginUser = (data: UserAuthenticationData): Promise<void> => { | ||
const { email, password } = data; | ||
|
||
if (email !== '[email protected]') throw new Error('Email does not exist!'); |
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'm confused about why do we have login. Also why is it hardcoded?
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.
It is a skeleton for an API function call with hardcoded error handling to see how the components react. Will add the connection. Also, It would been good to have both access and refresh tokens for JWT authentication. I think currently only access token is available.
const GRAY_COLOR = 'gray'; | ||
const RED_COLOR = 'red'; | ||
|
||
type Color = typeof BLUE_COLOR | typeof GREEN_COLOR | typeof GRAY_COLOR | typeof RED_COLOR; |
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.
Color should be an enum
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.
Hmm we will keep this solution so the Button color can be called by string
case SELECTOR_OPERATOR: | ||
return <OperationSelector onClose={onClose} addOperator={addOperator} />; | ||
default: | ||
return <OperationSelector onClose={onClose} addOperator={addOperator} />; |
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.
the default should throw an error
I get bunch of warnings when trying to start this with Failed to compile.
src/components/common/Inputs.tsx
Line 7:3: 'type' PropType is defined but prop is never used react/no-unused-prop-types
Line 7:3: propType "type" is not required, but has no corresponding defaultProps declaration react/require-default-props
Line 7:3: propType "type" is not required, but has no corresponding defaultProps declaration react/require-default-props
Line 8:3: propType "className" is not required, but has no corresponding defaultProps declaration react/require-default-props
Line 8:3: propType "className" is not required, but has no corresponding defaultProps declaration react/require-default-props
Line 9:3: propType "placeholder" is not required, but has no corresponding defaultProps declaration react/require-default-props
Line 9:3: propType "placeholder" is not required, but has no corresponding defaultProps declaration react/require-default-props
Line 24:3: propType "className" is not required, but has no corresponding defaultProps declaration react/require-default-props
Line 28:3: A form label must be associated with a control jsx-a11y/label-has-associated-control
Line 55:25: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 55:47: 'Checkbox' is assigned a value but never used @typescript-eslint/no-unused-vars
Line 69:25: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 69:47: 'Textarea' is assigned a value but never used @typescript-eslint/no-unused-vars
src/components/common/Lists.tsx
Line 10:65: 'ListHeaderAddRemove' is assigned a value but never used @typescript-eslint/no-unused-vars
Line 11:3: 'React' must be in scope when using JSX react/react-in-jsx-scope
Line 12:5: 'React' must be in scope when using JSX react/react-in-jsx-scope
Line 14:5: 'React' must be in scope when using JSX react/react-in-jsx-scope
Line 15:7: 'React' must be in scope when using JSX react/react-in-jsx-scope
Line 16:7: 'React' must be in scope when using JSX react/react-in-jsx-scope
src/components/pages/query/QueryBase.tsx
Line 2:10: 'Pql' is defined but never used @typescript-eslint/no-unused-vars
src/components/pages/query/QueryClosableContainer.tsx
Line 5:1: `semantic-ui-react` import should occur before import of `../../common/Buttons` import/order
src/components/pages/query/builder/PqlPipeline.tsx
Line 1:17: 'useEffect' is defined but never used @typescript-eslint/no-unused-vars
Line 1:28: 'useState' is defined but never used @typescript-eslint/no-unused-vars
Line 57:27: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 60:22: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 76:29: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 83:26: Curly braces are unnecessary here react/jsx-curly-brace-presence
src/components/pages/query/builder/loaders/EthereumBalanceLoader.tsx
Line 2:10: 'BlockType' is defined but never used @typescript-eslint/no-unused-vars
Line 40:22: Arrow function should not return assignment no-return-assign
Line 41:24: Arrow function should not return assignment no-return-assign
Line 42:22: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 42:22: Arrow function should not return assignment no-return-assign
Line 42:55: Missing radix parameter radix
Line 43:35: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 43:35: Arrow function should not return assignment no-return-assign
Line 44:34: Missing radix parameter radix
src/components/pages/query/builder/loaders/EthereumFunctionLoader.tsx
Line 4:10: 'BlockType' is defined but never used @typescript-eslint/no-unused-vars
Line 5:10: 'Button' is defined but never used @typescript-eslint/no-unused-vars
Line 57:22: Arrow function should not return assignment no-return-assign
Line 58:25: Arrow function should not return assignment no-return-assign
Line 59:24: Arrow function should not return assignment no-return-assign
Line 60:22: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 60:22: Arrow function should not return assignment no-return-assign
Line 60:55: Missing radix parameter radix
Line 61:35: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 61:35: Arrow function should not return assignment no-return-assign
Line 62:34: Missing radix parameter radix
Line 64:23: Arrow function should not return assignment no-return-assign
Line 66:45: Arrow function should not return assignment no-return-assign
Line 69:16: Do not use Array index in keys react/no-array-index-key
src/components/pages/query/builder/loaders/HttpGetLoader.tsx
Line 26:20: Arrow function should not return assignment no-return-assign
src/components/pages/query/builder/loaders/HttpPostLoader.tsx
Line 5:10: 'Button' is defined but never used @typescript-eslint/no-unused-vars
Line 37:20: Arrow function should not return assignment no-return-assign
Line 39:23: Arrow function should not return assignment no-return-assign
Line 41:54: Arrow function should not return assignment no-return-assign
Line 44:16: Do not use Array index in keys react/no-array-index-key
src/components/pages/query/builder/loaders/PostgresLoader.tsx
Line 30:20: Arrow function should not return assignment no-return-assign
Line 31:22: Arrow function should not return assignment no-return-assign
src/components/pages/query/builder/operators/AggregateOperator.tsx
Line 37:27: Arrow function should not return assignment no-return-assign
Line 38:28: Arrow function should not return assignment no-return-assign
Line 39:27: Arrow function should not return assignment no-return-assign
src/components/pages/query/builder/operators/GetIndexOperator.tsx
Line 25:22: Arrow function should not return assignment no-return-assign
Line 25:64: Missing radix parameter radix
src/components/pages/query/builder/operators/MathOperator.tsx
Line 34:28: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 34:28: Arrow function should not return assignment no-return-assign
Line 35:30: Arrow function should not return assignment no-return-assign
Line 36:31: Arrow function should not return assignment no-return-assign
src/components/pages/query/builder/operators/QuerySqlOperator.tsx
Line 2:20: 'Input' is defined but never used @typescript-eslint/no-unused-vars
Line 33:27: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 33:27: Arrow function should not return assignment no-return-assign
Line 34:28: Arrow function should not return assignment no-return-assign
Line 35:28: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 35:28: Arrow function should not return assignment no-return-assign
src/components/pages/query/builder/operators/TraverseOperator.tsx
Line 4:10: 'Button' is defined but never used @typescript-eslint/no-unused-vars
Line 5:17: 'Label' is defined but never used @typescript-eslint/no-unused-vars
Line 28:23: Arrow function should not return assignment no-return-assign
Line 30:45: Arrow function should not return assignment no-return-assign
Line 33:16: Do not use Array index in keys react/no-array-index-key
src/components/pages/query/selectors/LoaderSelector.tsx
Line 16:26: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 17:27: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 18:28: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 19:29: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 20:30: Missing return type on function @typescript-eslint/explicit-function-return-type
src/components/pages/query/selectors/OperationSelector.tsx
Line 16:27: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 17:27: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 18:23: Missing return type on function @typescript-eslint/explicit-function-return-type
Line 19:27: Missing return type on function @typescript-eslint/explicit-function-return-type
src/state/pql/pql.ts
Line 1:10: 'EOPNOTSUPP' is defined but never used @typescript-eslint/no-unused-vars What am I missing? |
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.
1st half of the review, will do the other half later on , but good job so far 👍
const logout = (): void => setUser({ ...emptyUser }); | ||
const logout = (): void => { | ||
setUser({ ...emptyUser }); | ||
window.location.pathname = LOGIN_PAGE_ROUTE; |
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.
should we use history.push ? like built in navigation with react ?
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.
Yes we should, but to access the history we need to place the logout inside the BrowserRouter, else it is undefined
src/api/pql.ts
Outdated
.then(() => axiosInstance.get<PQLWithHash>(`/ipfs/${hash}`)) | ||
.then((res) => res.data); | ||
|
||
// TODO Move those two interfaces somewhere |
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.
Should we have a folder for interfaces ? those are like harmless to have in one folder in dependencies
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.
Hmm I'm more thinking to replace them and use { result: any }
when applying the APICall
} | ||
|
||
export const loadIPFSWithHash = (hash: string): Promise<PQLWithHash> => | ||
Promise.resolve() |
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.
Why start with Promise.resolve ?
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 just my preference so that the chain starts with.then
, we can skip it if you do not like it
export const Button: React.FC<Button> = ({ children, onClick, color = GRAY_COLOR, className = '' }) => ( | ||
<button | ||
type="button" | ||
className={`focus:outline-none text-${color}-600 text-sm py-1.5 px-3 rounded-md border border-${color}-600 hover:bg-${color}-50 ${className}`} |
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.
👍 like the color here
src/components/common/Inputs.tsx
Outdated
} | ||
|
||
export const Label = ({ name, className = '' }: Label): JSX.Element => ( | ||
<label className={`my-auto text-lg font-medium text-gray-700 mr-3 ${className}`} htmlFor=""> |
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.
Should we add a conditionnal htmlFor in the arguments ? Maybe better to have the for in case
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.
Maybe we can add the argument with the default value
<div className="flex-1 flex items-center justify-center sm:items-stretch sm:justify-start"> | ||
<div className="flex-shrink-0 flex items-center"> | ||
<img className="block lg:hidden h-8 w-auto" src="/assets/images/logo-icon.png" alt="Workflow" /> | ||
<img className="hidden lg:block h-8 w-auto" src="/assets/images/logo-icon.png" alt="Workflow" /> |
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's the difference in between the 2 ? Or is it just for later so we can load two logos based on sreen size ?
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 a copy from tailwindui. didn't play around with it much so will try to remove it
|
||
export interface Pql { | ||
name: string; | ||
psql_version: string; // TODO suggestion: changed from snake cased to camel cased! |
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.
agree ! 👍
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 needs to be taking care of on BE
interface UrlParams { | ||
hash: string; | ||
} | ||
|
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 general statement,
Let's add some comments on each of the pages and big logic files on what we try to achieve within it,
the more we comment, the more it's accessible to anyone ( it's also easier to review )
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.
Well, my theory about the comments is to comment only on the things that are complicated or on the solutions that are not in the standard form (for example QueryConfig refresh callback). The code should have an understandable meaning by itself and must be written in a readable way with the use of good namings.
https://www.youtube.com/watch?v=7EmboKQH8lM&t=4043s&ab_channel=UnityCoin
}, [hash]); | ||
|
||
return error.length > 0 ? ( | ||
<ErrorContainer message={error} hash={hash} /> |
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.
For the error we might turn into the notification type of error. Also think we should keep the layout when displaying the error
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.
Agree this was the quick rewritten solution for the previous error handler
} | ||
|
||
const OperationConfig = ({ onClose, operator }: OperationConfig): JSX.Element => { | ||
// Because our operators are not common components, when their state is internally updated |
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.
👍
|
||
type UpdateData = (data: QueryData) => void; | ||
|
||
// This function needs to be take care off... |
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.
Yup good idea having it maybe in a util that just returns the new state and then we can setData inside the comp ( this way we can as well add a small unit test )
// If the end operator is not in the current source, find index will return -1, | ||
// therefore, we need to update it to the full length of operators | ||
// because of the next filter method | ||
if (endOperatorIndex === -1) endOperatorIndex = source.operators.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.
source.operators.length - 1
?
also should we bullet proof if source.operators empty we should have 0 there ?
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, it should be -1! will repair it. But still, because of the filter method, this change will not affect the functionality.
The source is never empty. It always has the loader operator inside, else it does not exist.
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 looks good, nice one! I like how you can run separate steps, see the result and then react accordingly. It needs a bit of UX improvement though. I was a bit confused at first how everything works and which buttons to press:
-
I would remove the initial "Pipeline 1", "Pipeline 2" JSON for the new PQL definitions made through the query builder. It makes sense for pure JSON editing (to see the example) but for the builder it just gets in the way as you have to remove the steps. I would replace the emptiness with a simple paragraph tutorial:
- "To add an API, click the "Loader" button
- "Click Run to execute the whole query and get the results"
- ... and so on, does not need to be over the top, just the simplest bits of information to get you started
-
Add a detailed hover text for every button on the toolbar: how does "Build"/"Compile"/"Run" differ from one another? And why should I care as a user? Shouldn't "Run" be all I need from the user perspective?
-
There was a lot of blank space on the left side when interacting with different components: the JSON editor becomes hidden and if an error occurs, nothing shows up. As a user, I am naturally confused what the heck is going on. Therefore I would have a result/status component always visible on top and then the JSON editor below (ditch the "Show code" button). Even if it is a strange looking error, dump it into the component as it is - no need for fancy formatting - and it will cause less frustration than the white-everything-is-fine type of screen.
-
When navigating away I can lose everything I made, if I haven't pressed the "Save" button. Add a popup: "Do you really want to leave the screen? Everything you made will be lost." Alternatively, if it is an easy feature to implement, save the structure to local storage whenever a change is made and designate such definitions as a draft.
-
(nice to have) For every loader/operator/aggregate edit/new menu we can show a simple example below the input fields to show the use of the parameters.
All my remarks are based from the user standpoint of view and are up for discussion. They do not need to be a part of this PR if so desired. Overall the functionality is amazing and very well done! 🚢 🇮🇹
|
|
A step-by-step tutorial would be great, but not sure if worth the effort. At the moment I would go for a popup with all the information to get you started. I can see this happening in two possible ways: The first popup would look something like this: Making PQL definitions is easy when using the query builder. We will create an example PQL that queries the CoinGecko for BTC price.
etc. at this point a user knows how to interact with most basic functionality and can play with other loaders/operators/aggregate. If an example of every operator is not provided when clicking on the operator (placeholder ...) we should list the reference for every operator/loader/aggregate function below: Operators: Traverse: allows to traverse the dictionary-like field (such as JSON) given a name. You can specify multiple fields to traverse multiple levels.
|
Defined QueryBuilder page with next content:
The main page container component is QueryLoader. Its purpose is to load the PQL statement from the URL, converting it and switching between Loading, Error, and QueryController states.
QueryController controllers the full page. It holds all the important states:
With the state information, it then connects QueryHeader, QueryBuilderHeader, Pql pipeline, QuerySelector, OperationConfig.
PQL state interface is defined in
state/pql/*
.The most important thing in this architecture is QueryData from query-builder.ts.
You can notice that the ExtendedOpeator uses Operator instance, which needs to be an object!
Because the operator is an object, values are not updated in React style with setState(), but are rather mutated internally.
The structure is prepared in a Builder pattern from objective-oriented (OO) programming principles.
This allows us to easily create new operators without changing the current structure.
From the structure can be also seen that the AggregationOperator is an "outside" operator. It is a bit hacky solution, which needs to be modified in the future (when we will add validation and some other stuff at the end of the pipeline). But the current structure satisfies functionality for the current PQL structure.
Loaders (
src/pages/query/builder/loaders/*
):Operators(
src/pages/query/builder/operators
):To update the Operator parameters OperatorConfig component has refresh functionality, which allows us to pass the refresh function callback into the Operator and it can trigger it every time the change is made.