Add query variable in react-scenes #818
Conversation
| <Stack direction="column"> | ||
| <Stack> | ||
| <VariableControl name="job2" /> | ||
| <Button variant="secondary" onClick={() => setHide(VariableHide.hideLabel)}> |
There was a problem hiding this comment.
Added a lot of buttons here to toy around with state here but will ultimately remove most of them for the demo.
| } | ||
|
|
||
| useEffect(() => { | ||
| if (variableAdded) { |
There was a problem hiding this comment.
@torkelo I'm not sure how to best tackle this or if it's something that we want. We need a way to refresh the variable when state changes, especially around value changes e.g: sort, regex which should trigger getValueOptions. I didn't find a way to easily do this other than re-adding the variable and letting it run it's activation, but that might not be the best approach. Simply setting the variable state won't work. Any ideas?
There was a problem hiding this comment.
@mdvictor yea, we might need some updates to QueryVariable to be able to re-evaluate options when sort / regex changes or re-issue query when query changes
|
Also, maybe we should add a prefix to these variables to delimit them from their scenes counterparts. |
@mdvictor it's own of the reasons we have separate package to allow some name conflicts, personally I really dislike the name conflicts, tried to get help with naming suggestions (around renaming or prefixing / suffixing) but did not get much feedback on options. |
| } | ||
|
|
||
| useEffect(() => { | ||
| if (variableAdded) { |
There was a problem hiding this comment.
@mdvictor yea, we might need some updates to QueryVariable to be able to re-evaluate options when sort / regex changes or re-issue query when query changes
| variable?.setState({ | ||
| label, | ||
| query, | ||
| datasource, | ||
| refresh, | ||
| sort, | ||
| regex, | ||
| value: initialValue, | ||
| isMulti, | ||
| hide, | ||
| includeAll, | ||
| }) | ||
| } |
There was a problem hiding this comment.
better to handle this state update in a separate useEffect that has some deep quality checks as this will not always trigger remove and re-add whenever this re-renders unless parent is very careful to memorize all props.
see useQueryRunner for reference
| }, [variable, scene, name]); | ||
|
|
||
| useEffect(() => { | ||
| variable?.setState({ |
There was a problem hiding this comment.
need a check here so this is not run on first mount
| private _handleVariableUpdate(variable: SceneVariable) { | ||
| this._variablesToUpdate.add(variable); | ||
|
|
||
| this._updateNextBatch(); | ||
| } |
There was a problem hiding this comment.
I think it's better to do this in a separate PR and not sure we can do it like this, not all state updates should cause a full update like this (would likely cause infinite update loops)
There was a problem hiding this comment.
Sounds good, I'll move this to another PR. Yeah, it's risky. I think it worked because of this but I might be missing something. In any case, I changed this to be called manually.
| private _updateOptionsOnStateChange = (newState: QueryVariableState, oldState: QueryVariableState) => { | ||
| if (!isEqual(pick(newState, this._stateChangePropsToCompare), pick(oldState, this._stateChangePropsToCompare))) { | ||
| this.publishEvent(new SceneVariableStateChangeEvent(this), true); | ||
| } | ||
| }; |
There was a problem hiding this comment.
I think it's better to give consumer manual control over which state updates should trigger a re-eval of options
There was a problem hiding this comment.
I agree. Added a method in MultiValueVariable as it will need to be called in other variables as well (custom/datasource). Will make a new PR with these changes.
| @@ -329,6 +330,10 @@ export abstract class MultiValueVariable<TState extends MultiValueVariableState | |||
| return options; | |||
| } | |||
|
|
|||
| public refreshOptions() { | |||
There was a problem hiding this comment.
Added this func to update options after the state has changed.
| variable.setState({ | ||
| label, | ||
| query, | ||
| datasource, | ||
| refresh, | ||
| sort, | ||
| regex, | ||
| hide, | ||
| includeAll, | ||
| }); | ||
|
|
||
| variable.refreshOptions(); |
There was a problem hiding this comment.
because query and data source are objects, I would do a deep equals diff here to make sure thing have actually changed
|
🚀 PR was released in |
Adds the Query Variable type to react-scenes
Depends on #827