-
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
OperationEditor: Fix rendering of operation editor items #112
Conversation
const onParamValueChanged = (paramIdx: number, value: QueryBuilderOperationParamValue) => { | ||
const update: QueryBuilderOperation = { ...operation, params: [...operation.params] }; | ||
update.params[paramIdx] = value; | ||
callParamChangedThenOnChange(definition, update, index, paramIdx, onChange); | ||
}; | ||
|
||
const onAddRestParam = () => { | ||
const update: QueryBuilderOperation = { ...operation, params: [...operation.params, ''] }; | ||
callParamChangedThenOnChange(definition, update, index, operation.params.length, onChange); | ||
}; | ||
|
||
const onRemoveRestParam = (paramIdx: number) => { | ||
const update: QueryBuilderOperation = { | ||
...operation, | ||
params: [...operation.params.slice(0, paramIdx), ...operation.params.slice(paramIdx + 1)], | ||
}; | ||
callParamChangedThenOnChange(definition, update, index, paramIdx, onChange); | ||
}; |
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 would conside useCallback
these, now that we're at it, as it doesn't need to change the children on every render.
let restParam: React.ReactNode | undefined; | ||
if (definition.params.length > 0) { | ||
const lastParamDef = definition.params[definition.params.length - 1]; | ||
if (lastParamDef.restParam) { | ||
restParam = renderAddRestParamButton(lastParamDef, onAddRestParam, index, operation.params.length, styles); | ||
} | ||
} |
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 useMemo
this.
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.
LGTM!
I have introduced an issue when I was refactoring OperationEditor (at that time in
grafana/grafana
, but because I wasn't passing props correctly to component, it was doing a lot of mounting - unmounting grafana/grafana@7be8301. I will have to do this fix also ingrafana/grafana
This PR takes previously called
StyledOperationHeader
and moves it to separate file and makes it a proper component that doesn't cause -remounting of each operation editor.Fixed:
fixed.mov
Broken:
broken.mov
How to test:
yarn link
and test this locally, do that.package.json
tocustom
grafana/grafana
package.json
add"custom": "file: ../grafana-experimental"
OperationList
in Loki fromcustom