-
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
Add embed mode options in the Share UI #58435
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
Jenkins, test this |
Pinging @elastic/kibana-app (Team:KibanaApp) |
src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx
Outdated
Show resolved
Hide resolved
@@ -348,7 +348,7 @@ function QueryBarTopRowUI(props: Props) { | |||
className={classes} | |||
responsive={!!props.showDatePicker} | |||
gutterSize="s" | |||
justifyContent="flexEnd" | |||
justifyContent="flexStart" |
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've made this change because the date picker looks out of place when it's right aligned and the query input is not displayed - mostly because the filter bar is left aligned below it. Is there any harm in doing this?
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 don't see any harm in doing this.
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 affects visualizations that hide the query bar (Timelion, TSVB). It's probably a matter of taste what's worse - moving the date picker from its regular place or having a right-aligned line in the layout @cchaos what do you think?
This is how TSVB looks with this change:
Personally I have a slight preference for flexEnd
, but no strong opinion
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 generally prefer the right alignment, and I feel keeping the date picker location consistent is a good thing. However, in embedded mode this may not be as important because the end user may not be a frequent Kibana user, and I think that it looks out of place when there is content on the lines above and below like this:
I can make this a prop so that it doesn't impact the other views, but it would be good to hear some more thoughts. I also have no strong opinion on this, but I made the change to find out if it's preferred. I'd be happy to change it back.
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 agree with you @ajwild. We are aware that this is not ideal and are continuing a search for a better solution. The better solution though involves lots of folks across different teams coming to a conclusion so it's taking a while to nail down. Consistency is the main factor here, and we should try to keep the global time filters in the same place across all apps (even when no query bar exists).
In embed mode 🤷♀ , it's probably okay to shift.
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.
@ajwild I like the prop solution in that case
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 it's okay to do a shift, as long as it's not complicating the API \ code.
As the design of this will soon change anyway, I think we could go with an 80:20 solution, allowing having the datepicker in embedded mode, even if it's not perfectly positioned ATM.
src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx
Outdated
Show resolved
Hide resolved
['&show-top-nav-menu=true', this.state.showTopNavMenu], | ||
['&show-query-input=true', this.state.showQueryInput], | ||
['&show-date-picker=true', this.state.showDatePicker], | ||
['&hide-filter-bar=true', !this.state.showFilterBar], // Inverted to keep default behaviour for old links |
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.
Can we name all parameters hide-something
for consistency?
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'd prefer show, just so we're consistent with our own flags.
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 I change it to use &show-filter-bar=true
, then all previously generated links (which users may have already embedded into websites etc.) will begin hiding the filter bar because this value won't be set.
Edit: This would also be the case if switching to &hide-*
for the other parameters - it depends what default are wanted for older links. An alternative approach would be to implement =false
, and then if the value is missing then the current defaults could be used.
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.
Well, then lets go for hide-something
like @majagrubic suggested?
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 for deciding to contribute to Kibana. I've tested this and it works well. Some overall comments:
-
This will introduce the new options in all the places where
UrlPanelComponent
is used, not just the dashboard - eg. Visualize, and since Visualize code hasn't been changed to make use of these parameters, the embeddable renders it without nav. I think we should add a dashboard-specificUrlPanelComponent
and make this change for Dashboard only. -
The "showFilter" switch doesn't seem to work properly - regardless if it's switched on/off the url parameter is
hide-filter-bar=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.
That's great @ajwild - thanks for looking into this! I added a few suggestions, but this is looking pretty good already.
@@ -348,7 +348,7 @@ function QueryBarTopRowUI(props: Props) { | |||
className={classes} | |||
responsive={!!props.showDatePicker} | |||
gutterSize="s" | |||
justifyContent="flexEnd" | |||
justifyContent="flexStart" |
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 affects visualizations that hide the query bar (Timelion, TSVB). It's probably a matter of taste what's worse - moving the date picker from its regular place or having a right-aligned line in the layout @cchaos what do you think?
This is how TSVB looks with this change:
Personally I have a slight preference for flexEnd
, but no strong opinion
How about adding a label over the "Show" toggle buttons to reduce the amount of text the user needs to read. Here is my suggested wording: Include Top menu Also, for these four items, I don't feel a tooltip is needed. |
I agree with @gchaps. And at that point, the group should just be a Let me know if you'd like help implementing these UI controls. |
Thanks for all the feedback! I hope to make the changes requested on Monday, and I'll reach out if any questions come up. |
bbca5a7
to
0632065
Compare
@elasticmachine merge upstream |
7a39762
to
9c977f1
Compare
9c977f1
to
417abf0
Compare
@flash1293 it wasn't a misunderstanding, it was something I overlooked when refactoring (see #58435 (comment)). It should be working as expected now. I've rebased and fixed a merge conflict too. |
@elasticmachine merge upstream |
Jenkins, test this |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
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.
Tested in Chrome and Firefox, LGTM. Thanks a lot @ajwild for this great contribution!
No problem! Thanks to you and everyone else involved for all the helpful feedback and patience. |
# Conflicts: # src/plugins/dashboard/public/application/embeddable/viewport/dashboard_viewport.tsx
* master: [ML] DFAnalytics results: ensure ml result fields are shown in data grid (elastic#68305) [security_solution] enable react-hooks/exhaustive-deps (elastic#68470) Closes elastic#66867 by adding missing, requried API params (elastic#68465) [Telemetry] collect number of visualization saved in the past 7, 30 and 90 days (elastic#67865) [Logs UI] View in context tweaks (elastic#67777) Kibana developer examples landing page (elastic#67049) Bump decompress package version (elastic#68386) fix elastic#66185 (elastic#66186) Bump pdfmake package version (elastic#68395) Unskip embeddables/adding_children suite (elastic#68111) Add embed mode options in the Share UI (elastic#58435) Adding key to avoid react warning (elastic#68491)
If you don't include the time filter, what time is used? Is the current time range from the URL included? Does the iFrame user know what the time range is? If it's a relative time range does that still work? |
@LeeDr It's using the time range from the url (which can also be relative by using datemath involving The time range is not shown anywhere to the iframe user, but that's just how it worked all the time. |
Amazing news guys, these features are deal breakers for us! |
@RonnieDilli it will be released in the upcoming version 7.9 |
Great new feature! |
Amazing future. I'm waiting for this release. |
Hello, pushing that up. Any news on when this will be merged? thanks |
@pgraca-38 it's already released with 7.9 |
Thank You! Completly missed it ) |
Summary
This addresses a few of the capabilities listed in #9575 - specifically showing the query input, date picker and filters. I haven't looked into the zooming and panel moving/resizing options yet.
I also made it possible to hide the top nav menu items (Full Screen, Share, Clone, Edit) but this exposed a bug so it may be simpler to remove this functionality if it's not really required.
I'll add a few comments below where some feedback would be appreciated.
Checklist
Delete any items that are not applicable to this PR.
For maintainers