-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Fix embed view #5166
Fix embed view #5166
Conversation
panda01
commented
Oct 21, 2015
- Removed collapsible sidebar.
- Removed spy panel.
…lize closes elastic#5165. Added a chrome.getVisible to spy panel closes elastic#3820
I don't think the spy panel should be removed from the embed mode. |
Whoops, silly me. Fixed that @BigFunger. |
@panda01 Your most recent commit re-added the spy tab to the visualization editor in the Visualize app, but it is still missing from the Discover app and the Dashboard app. |
1225090
to
3438cd2
Compare
LGTM The only concern that I have is the name of the chrome property that you're taking advantage of. @spalger is the name of that property descriptive enough given the context in which it is being used here? I assume that the getVisible function refers to the visibility of the chrome-application wrapper as a whole. Since the spy panel is a part of something other than the chrome-application wrapper (namely a visualization), should there be some other mechanism in play here? Or are we OK with arbitrarily deciding what elements are part of the chrome and what are not? |
@BigFunger I'm fine deciding whether or not the spy is visible based on if the chrome is visible. The intention behind |
I'm not really that much of a fan of that either. I think perhaps it would
|
@panda01 pass in the function? Not sure what you mean. |
You mean pass in an |
No I mean pass in the value. Of the function. As a param like
|
Yeah, at the visualization level I think that makes more sense |
7531698
to
eba2c44
Compare
…lt if no option is passed in for show-spy-panel Removed conditional on from discover
LGTM! |
LGTM, but needs a merge with master |
This was never backported to 4.2 |