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

port hideParameters dashboard param upstream #2948

Closed
wants to merge 1 commit into from

Conversation

alison985
Copy link
Contributor

Original issue: mozilla#163
Issue to port upstream: mozilla#563

This allows you to add ?hideParameters=true to a dashboard to hide them in widgets.

Without this parameter:
screenshot 2018-10-14 19 14 36

With parameter:
screenshot 2018-10-14 19 17 10

@jezdez
Copy link
Member

jezdez commented Oct 16, 2018

@arikfr I realize this is a small feature and a bit of a hack, mozilla#163 has some more info about this:

First I will describe the use-case here. If I'm generating dashboards via the API, the templating mechanism is useful to me to set variables for a query given the data my redash client has. However, once I've set them and generated a dashboard, I don't want it to be possible for these to be easily changed and I don't want to have those text boxes in the dashboard.

-- @emtwo

@jezdez jezdez closed this Oct 16, 2018
@jezdez
Copy link
Member

jezdez commented Oct 16, 2018

Woops.

@jezdez jezdez reopened this Oct 16, 2018
@arikfr
Copy link
Member

arikfr commented Oct 16, 2018

@jezdez what I wonder is whether it's needed post #2756 (note that "Static Parameter Value" functionality). The question is whether in @emtwo's use case she generates for each set of parameter values a new dashboard or reuses the same one?

@emtwo
Copy link

emtwo commented Oct 16, 2018

@arikfr a dashboard would have its own set of parameter values and typically wouldn't get updated.

I do think that aside from the auto-dashboard generation, it's useful to be able to remove the parameter textboxes in a dashboard so it's more presentable. So perhaps it could have a "presentation mode" without the parameter text boxes and an "edit mode" with the text boxes.

I'm not sure if that's what the "Static Parameter Value" functionality that you mentioned does (didn't read through that whole PR)

@arikfr
Copy link
Member

arikfr commented Oct 18, 2018

I'm not sure if that's what the "Static Parameter Value" functionality that you mentioned does (didn't read through that whole PR)

Among the other things implemented in that PR, it also implements the ability to set a static value to a parameter when adding the query/widget to the dashboard. If you create static dashboards based on queries with parameters, this seems like what you need. When a static value is used, then no UI is rendered for it.

@alison985
Copy link
Contributor Author

@arikfr I don't see any API functionality mentioned in #2756 so I don't think that PR will address the use case this was implemented to handle.

@arikfr
Copy link
Member

arikfr commented Oct 28, 2018

@alison985 why would you expect to see API functionality changes?

@alison985
Copy link
Contributor Author

@arikfr When creating a dashboard via the API, how else would you set a parameter to "Add the parameter to the dashboard", "Map to existing parameter", or "Use static value for the parameter'? Or set the static value of a parameter for a widget?

@emtwo
Copy link

emtwo commented Oct 29, 2018

I guess what @alison985 means (correct me if I'm wrong) is that we were creating dashboards with the API and using the parameters. These static parameters sound like they will behave correctly in the UI - they won't show the textboxes. However, we won't be able to use static parameters via the API. Is this true?

@arikfr
Copy link
Member

arikfr commented Oct 29, 2018

@alison985 @emtwo of course the API supports this, as otherwise the interface couldn't use this :) No API changes were required as these are just options we store in the Widget.options blob.

@rafrombrc
Copy link

Digging into this a bit, it does look like #2756 will suffice to meet the use case for which this change was originally intended. #2756 hasn't been merged yet, but still we'll retract this PR, we can reopen it if we need to revisit.

@jezdez
Copy link
Member

jezdez commented Nov 8, 2018

Thanks!

@jezdez jezdez closed this Nov 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants