-
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
[Dashboard Navigation] Drilldown on link click #164196
[Dashboard Navigation] Drilldown on link click #164196
Conversation
c1242f1
to
ff2f699
Compare
ff2f699
to
c367b7b
Compare
a134f3d
to
f7e8309
Compare
cb1f664
to
ec4b98e
Compare
b9c8302
to
3953b4f
Compare
5f5e21f
to
4c862e4
Compare
4c862e4
to
4712e57
Compare
ca0ba7c
to
185f507
Compare
/oblt-deploy |
Pinging @elastic/kibana-presentation (Team:Presentation) |
}} | ||
initialSelection={externalLinkDestination} | ||
onDestinationPicked={(url) => setDestination(url, url)} | ||
setDestinationError={setDestinationError} |
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.
Only external links currently have validation - you cannot select an invalid dashboard and, even if you are editing an existing link where the selected destination dashboard was deleted, we simply treat this as "no selection" rather than "invalid selection" . After all, the panel editor already tells the user that the dashboard was deleted - IMO, we don't need to duplicate this information in the link editor:
…-ref HEAD~1..HEAD --fix'
import { NavEmbeddableStrings } from '../navigation_embeddable_strings'; | ||
import { EuiFormRow } from '@elastic/eui'; | ||
|
||
export const NavigationEmbeddableLinkDestination = ({ |
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 separated this out into a new component for two reasons:
- It cleans up the logic of
NavigationEmbeddableLinkEditor
a little bit - It makes it easier to remember selections even when the link types change, like so:
Before | After |
---|---|
I opted to do this because the link options remembered their previous selections (which I figured was important, since we don't want users to accidentally lose their settings), so it felt inconsistent that the destination wouldn't be remembered.
tooltipTitle: Boolean(dashboardDescription) ? linkLabel : undefined, | ||
tooltipMessage: dashboardDescription || linkLabel, | ||
}; | ||
}, [error, dashboardTitle, dashboardDescription]); | ||
}, [error, linkLabel, dashboardDescription]); |
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.
Figured I might as well quickly make this change per the offline tooltip description, since it's such a tiny change - previously, we were always showing the saved object title in the tooltip; now, we default to the custom label, if provided. This means that the link label and the tooltip will always display the same value
src/plugins/navigation_embeddable/common/content_management/v1/cm_services.ts
Show resolved
Hide resolved
if (allowedUrl === null) { | ||
throw new DisallowedUrlError(); | ||
} | ||
const validatedUrl = urlDrilldownValidateUrl(url); |
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.
Are these the only protocols we will support (http, https, mailto)? I may need to update my content management schemas accordingly. https://github.com/elastic/kibana/pull/164330/files#diff-e2bc370404a35f02472741df329913c28531da5e3ef140b2067bb4ba69ccc465R40
...act_dashboard_drilldown/components/dashboard_drilldown_config/dashboard_drilldown_config.tsx
Outdated
Show resolved
Hide resolved
...act_dashboard_drilldown/components/dashboard_drilldown_config/dashboard_drilldown_config.tsx
Outdated
Show resolved
Hide resolved
…bstract_dashboard_drilldown/components/dashboard_drilldown_config/dashboard_drilldown_config.tsx Co-authored-by: Nick Peihl <[email protected]>
…bstract_dashboard_drilldown/components/dashboard_drilldown_config/dashboard_drilldown_config.tsx Co-authored-by: Nick Peihl <[email protected]>
const destination = useMemo(() => { | ||
return linkOptions.encodeUrl ? encodeURI(link.destination) : link.destination; | ||
}, [linkOptions, link.destination]); | ||
|
||
return ( | ||
<EuiListGroupItem |
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 we still need to validate the URL again here against the externalUrl policy. It is possible for someone to create an external link to https://example.com. But later the Kibana admin decides to set an external policy against that domain. In that case, the link should probably be disabled with a warning about the external link policy.
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.
Ahhh, very good point 👍 Will do
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 in 3ca397b!
Screen.Recording.2023-08-31.at.4.12.38.PM.mov
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.
🚀 LGTM, great job!
…o nav-link-drilldown_2023-08-16
…kibana into nav-link-drilldown_2023-08-16
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.
lgtm! code review and tested dashboard and external links with all options.
src/plugins/navigation_embeddable/public/components/dashboard_link/dashboard_link_component.tsx
Outdated
Show resolved
Hide resolved
…link/dashboard_link_component.tsx Always show tooltips on Dashboard Links
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @Heenawter |
Closes #154381
Summary
This PR adds functionality to the link embeddable so that
Dashboard Links
Screen.Recording.2023-08-28.at.11.15.37.AM.mov
Link settings
We want dashboard drilldowns and dashboard links to work more-or-less the same way, which means that they should have identical settings - therefore, in order to share the same settings between dashboard drilldowns + dashboard links, I created the
DashboardDrilldownOptionsComponent
:I put this new component into
presentation_util
so that it could easily be shared and used in the respective editing experiences.Testing
ctrl + click
,shift + click
, andmeta + click
work as expected, regardless of whether theOpen in new tab
setting is true or false - specifically, we should use the URL to pass the filters/queries/time range whenever a dashboard is being opened in a new tab.URL Links
Screen.Recording.2023-08-28.at.11.33.49.AM.mov
Link settings
Similar to the dashboard links, we want the
Testing
When testing, it is important to ensure that the new URL validation works as expected. For this, there are technically two types of invalid links:
http
,https
, ormailto
.externalUrl.policy
Testing number one should be simple; however, in order to test the second type of invalid links, you need to modify your
kibana.dev.yml
to include something like the following:Refer to http://www.elastic.co/guide/en/kibana/current/url-drilldown-settings-kb.html for more information.
Also, much like with the dashboard links, it is important to ensure that
ctrl + click
,shift + click
, andmeta + click
work as expected, regardless of whether theOpen in new tab
setting is true or false.Checklist
Unit or functional tests were updated or added to match the most common scenariosWill be addressed in [Dashboard Navigation] Add functional + unit tests #161287For maintainers