Skip to content
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

Possibility to refresh variable options based on state changes #827

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

mdvictor
Copy link
Collaborator

@mdvictor mdvictor commented Jul 11, 2024

Add possibility to manually trigger a variable options refresh after a state change. Needed for various variable components in react scenes. E.g.: #818

📦 Published PR as canary version: 5.7.5--canary.827.10385100405.0

✨ Test out this PR locally via:

npm install @grafana/scenes-react@5.7.5--canary.827.10385100405.0
npm install @grafana/scenes@5.7.5--canary.827.10385100405.0
# or 
yarn add @grafana/scenes-react@5.7.5--canary.827.10385100405.0
yarn add @grafana/scenes@5.7.5--canary.827.10385100405.0

@mdvictor mdvictor added patch Increment the patch version when merged release Create a release when this pr is merged labels Jul 11, 2024
@mdvictor mdvictor requested a review from torkelo July 11, 2024 07:43
@mdvictor mdvictor requested a review from dprokop July 12, 2024 10:30
Comment on lines 264 to 268
private _handleVariableOptionsUpdate(variable: SceneVariable) {
this._variablesToUpdate.add(variable);

this._updateNextBatch();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't think this is needed, The variable can handle this and if the value changes due to options changes then it should emit the event for variable value change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but if the current variable value is a single value then updating options does not change anything (no need to update other variables).

Copy link
Collaborator Author

@mdvictor mdvictor Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need it because the values aren't changing, only the options or options related properties. For example if the options sorting changes we need to update that var and show the options asc/desc in the UI, but the value itself does not change.

I don't understand how variable value change would help here, I might be missing some piece of the puzzle.

Copy link
Member

@torkelo torkelo Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdvictor I just don't understand why the set needs to handle this, this concern of updating options based on sorting etc is fully the responsibility of the variable.

the set is only concerned if the value changes and if that should trigger cascading updates to other variables, and for that we have the SceneVariableValueChangedEvent event

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I understand what you mean now, yeah that makes sense, I'll move this for the variable itself to handle.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@torkelo I modified this to be handled by the variable itself, definitely better

@mdvictor mdvictor requested a review from torkelo August 13, 2024 09:53
@mdvictor mdvictor merged commit 0236734 into main Aug 14, 2024
3 checks passed
@mdvictor mdvictor deleted the mdvictor/variable-refresh-options branch August 14, 2024 09:59
@grafanabot
Copy link
Contributor

🚀 PR was released in v5.7.5 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged release Create a release when this pr is merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants