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

Add method to get ancestor for localValueVariable #699

Closed
wants to merge 1 commit into from

Conversation

mdvictor
Copy link
Contributor

@mdvictor mdvictor commented Apr 17, 2024

In certain scenarios (e.g.: grafana/grafana#84915) where we repeat a panel under a repeated row using the same variable, the DashboardGridItem._performRepeat looks up and finds the variable in the grid row, since they have the same name, and since that is a LocalValueVariable it cannot perform the repeat with it.

With this method we can get the parent variable of the LocalValueVariable and use that to repeat.

Needed by grafana/grafana#86425

📦 Published PR as canary version: 4.11.1--canary.699.8720696583.0

✨ Test out this PR locally via:

npm install @grafana/scenes@4.11.1--canary.699.8720696583.0
# or 
yarn add @grafana/scenes@4.11.1--canary.699.8720696583.0

@mdvictor mdvictor added patch Increment the patch version when merged release Create a release when this pr is merged labels Apr 17, 2024
@torkelo
Copy link
Member

torkelo commented Apr 17, 2024

@mdvictor what would be the point of this scenario?

I do not think we should support it.

If the row is repeated by variable A, and you repeat a panel inside that row by the same variable it should only see the local scoped value.

That is how repeating works.

@mdvictor
Copy link
Contributor Author

@torkelo I'm also wondering if this should be supported. It might be an edge case to use the same var for both panels and rows.

My only argument for supporting this would be parity with the old arch, where you can have a dashboard with repeated panels inside repeated rows under the same variable and they will both update when that variable changes.

In the new arch the repeated panels will not update because the type is LocalValueVariable and it only accepts MultiValueVariable.

Do we not want to support this specific implementation or the scenario at all?

@torkelo
Copy link
Member

torkelo commented Apr 17, 2024

@mdvictor i would say that is a bug in the old architecture,

The query in a panel would get the local value, so the panel repeat should also

@mdvictor
Copy link
Contributor Author

mdvictor commented Apr 17, 2024

@torkelo Just making sure I understand right: We do not want to support the scenario of having a repeated row and a repeated panel under it using the same variable? Or we do but using the local value instead of searching up the three?

@mdvictor mdvictor closed this Apr 17, 2024
@torkelo
Copy link
Member

torkelo commented Apr 18, 2024

Just making sure I understand right: We do not want to support the scenario of having a repeated row and a repeated panel under it using the same variable?

@mdvictor , yea this scenarios does not make sense as the variable will only have single value under the row

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants