-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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: global parameters for dashboards #1504
Conversation
+1 for this feature following this post (https://discuss.redash.io/t/add-ui-for-query-params-at-dashboard-level/236/10). |
Not sure it's up to @arikfr, I haven't heard from him yet. |
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.
Thanks!
Looks good - the main thing I would appreciate if you change is the use of the event to sync parameters.
Another thing -- how about we add to the parameter configuration (the one that opens with the cog icon) whether it's global or not?
Using conventions for configuration is something we used in the early days, but I'm trying to avoid as it hurts feature discovery.
}); | ||
}; | ||
|
||
$scope.$on('deleteDashboardWidget', extractGlobalParameters); |
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.
Instead of using an event, let's add a callback function to the widget component.
}; | ||
|
||
this.onGlobalParametersChange = () => { | ||
_.each(this.globalParameters, (global) => { |
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.
You can use forEach
here and for global.locals
.
I kind of like the implicit, but I understand the importance of importance of feature discovery. I'll add it in. |
@arikfr added in the explicit toggle: https://goo.gl/photos/8HWUN7nYro6Xv8Qq5 |
@@ -293,6 +294,21 @@ function QueryResource($resource, $http, $q, $location, currentUser, QueryResult | |||
return this.getParameters().get(); | |||
}; | |||
|
|||
Query.prototype.getLocalParametersDefs = function getLocalParametersDefs() { | |||
if (!this.$localParameters) { | |||
this.$localParameters = this.getParametersDefs().filter(p => !p.global); |
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's safe to cache because it's used only in dashboards, where the query won't change - right?
Maybe we should move these two functions to the dashboard code, to avoid someone using it "by accident" in another context?
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 must be cached or angular can't detect that the state isn't changing. The normal parameters are cached, so why is this case any different?
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.
Normal parameters aren't cached -- whenever we call Parameters.get
it calls Parameters.updateParameters
(which is a no-op in case the query text didn't change). Or are we talking about different things?
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.
Oh I see. The parameters object is cached, but I didn't look into what get
did. I'll move it to dashboard.
@arikfr PTAL |
👌 thanks! :-) |
Whoop! Thank you |
Any variable prefixed with
$
will be elevated to a global param, check out the video below for a demo.https://goo.gl/photos/3eXQRnJFuoyh4TRs5
This PR: #1483 for the master branch and much cleaner. Good work on the refactor the new codebase is much cleaner than before!
Feature request here: https://discuss.redash.io/t/add-ui-for-query-params-at-dashboard-level/236/8
/cc @tonymeng