-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Added dashboard standalone page #1429
Conversation
@@ -1793,6 +1793,12 @@ def dashboard(self, dashboard_id): | |||
def dashboard(**kwargs): # noqa | |||
pass | |||
dashboard(dashboard_id=dash.id) | |||
if request.args.get("standalone") == "true": | |||
return self.render_template( | |||
"caravel/dashboard_standalone.html", |
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.
do we need to render a new template for the standalone view? or could we add some logic to basic.html
where if we are on the dashboard page and the standalone arg is true, don't show the header. we are doing something similar for the standalone viz's afaik.
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.
I saw that we have a separate standalone template for explore view: https://github.com/ascott/caravel/blob/f837733d858643152304469c899c096e31d2bf24/caravel/views.py#L1194
Apart from the header, I also took out the button groups, fav star to limit interactions on dashboard standalone page. We could have if statements in both basic.html and dashboard.html for rendering specific parts based on standalone argument, but I was thinking that having a separate template would be cleaner?
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.
i think keeping one template for the dashboard and adding the conditions where needed will be more clear in the long run since we won't have to maintain 2 templates. the conditional for show/hiding the header should work the same way in basic.html
for both the slice and dashboard view, so we will probably only need the conditionals in the dashboard template.
cc @johnbodley |
I vote for parameterizing the template we have now. |
3d68f35
to
bdd6a11
Compare
<button type="button" class="btn btn-default" data-dismiss="modal"> | ||
Close | ||
</button> | ||
{% if not dash_standalone %} |
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.
looks like this only needs to wrap the #add-slice-container
element, since the modal will only be shown if the button that triggers it clicked.
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.
Done:)
bdd6a11
to
bbb0e3e
Compare
@@ -21,7 +21,7 @@ | |||
|
|||
<body> | |||
{% block navbar %} | |||
{% if not viz or viz.request.args.get("standalone") != "true" %} | |||
{% if not dash_standalone and (not viz or viz.request.args.get("standalone") != "true") %} |
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.
looks like this should be if not dash_standalone or (not
rather than and
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.
If we change it to 'or', when dash_standalone=true, not dash_standalone = false, but viz is not passed in, so not viz will be true, which makes the if statement to be true and header is displayed.
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 could also have a more generic "show_navbar" argument and pass it explicitly in both views.
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.
Do we hide the navbar in any other cases besides explore-standalone? I think in views.py we render standalone.html when standalone=true in the url. Seems like {% if not viz or viz.request.args.get("standalone") != "true" %} will always be true, since standalone.html doesn't extend basic.html
last comment re: and/or. with that fix LGTM! |
LGTM! 🚢 👍 |
@vera-liu Hi Vera, it seems not working in Superset 0.13.0. |
turned out this was not added for reactified dashboard, https://github.com/airbnb/superset/pull/1596 should solved it. |
Thank you @vera-liu 👍 |
* fix: show value on the selected series * fix: callback
* fix: show value on the selected series * fix: callback
* fix: show value on the selected series * fix: callback
* fix: show value on the selected series * fix: callback
Feature request: #1354
Done:
When standalone=true is specified in url, a standalone dashboard page is rendered. This page doesn't have caravel header or button groups/fav star, it will be more convenient to make screenshots.
without standalone arg:
with standalone arg:
Todo:
I'm thinking about adding a button to our button group in dashboard view for getting standalone endpoint for dashboard, would this be useful?
needs-review @ascott @bkyryliuk @mistercrunch