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

[Dashboard Navigation] Make links work in portable dashboards #164748

Closed
Tracked by #154354
Heenawter opened this issue Aug 24, 2023 · 2 comments · Fixed by #167928
Closed
Tracked by #154354

[Dashboard Navigation] Make links work in portable dashboards #164748

Heenawter opened this issue Aug 24, 2023 · 2 comments · Fixed by #167928
Assignees
Labels
Feature:Dashboard Dashboard related features impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:large Large Level of Effort Project:Dashboard Navigation Related to the Dashboard Navigation Project Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects

Comments

@Heenawter
Copy link
Contributor

Heenawter commented Aug 24, 2023

Follow up to #154381

Describe the feature:
Currently, dashboard-to-dashboard links assume that the user is in the Dashboard app when navigating - specifically, we are exclusively using the dashboard app's locator. This means that, if there is a dashboard embedded in (for example) a security solution, any links panels will direct the user to the dashboard app rather than staying in the embedded security context.

In order to make the locator more generic, we need to instead have the dashboard container accept a locator as a prop, and use that instead - that way, the dashboard app can provide its specific locator to the container, while other solutions can provide their own locators as necessary. Then, the links embeddable should always be able to stay in the correct context when navigating.

As part of this, we will need to find every embedded dashboard and update the implementation to provide this new locator prop - this may mean defining locators for these solutions as well, since they may not have one yet. I think it would also be beneficial to add an embedded dashboard with a links panel to the developer examples.

@Heenawter Heenawter added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:medium Medium Level of Effort impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Project:Dashboard Navigation Related to the Dashboard Navigation Project labels Aug 24, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@Heenawter Heenawter changed the title [Dashboard Navigation] Make links work in embedded dashboards [Dashboard Navigation] Make links work in portable dashboards Aug 31, 2023
@Heenawter Heenawter added the Feature:Dashboard Dashboard related features label Aug 31, 2023
@kibanamachine kibanamachine added this to Inbox in Dashboard Aug 31, 2023
@ThomThomson ThomThomson added loe:large Large Level of Effort and removed loe:medium Medium Level of Effort labels Sep 14, 2023
@ThomThomson
Copy link
Contributor

Bumping this up to a large LOE because I think it might be more work than anticipated to add locators in Observability and security. There are also a number of other places where we navigate around solutions / editors etc without using locators that we'd need to be careful not to break.

@Heenawter Heenawter self-assigned this Sep 18, 2023
Heenawter added a commit that referenced this issue Nov 6, 2023
Closes #164748

## Summary

Previously, the link embeddable **always** used the Dashboard plugin's
locator - this meant that, for portable dashboards (such as those in
Security or in APM), navigation through the link embeddable would always
send the user to the Dashboard plugin rather than staying in whatever
context the portable dashboard was in. This PR fixes that by ensuring
that each `DashboardRenderer` consumer can provide their **own** locator
as a prop - this means that the Dashboard app can still use its own
locator, while the Security/APM plugins (for example) can **also**
define a locator so that navigation remains in the security/APM app.

### Security @elastic/security-solution-platform 

While I did my best to modify each portable dashboard implementation to
include this new locator prop, without the full context on how each
plugin wants to handle the link embeddable, I've had to make a few
assumptions about the behaviour. For example, in security....

- The Security dashboards **not** respond to the query bar in the same
way that they do in the Dashboard app - therefore, the link embeddable
settings for "Use filters and query from origin dashboard" and "Use date
range from origin dashboard" do not make sense in this context.

For example, in Security, it is (from my understanding) **expected**
that the query bar would always remain constant, regardless of these
settings; therefore, these settings are more-or-less ignored.
Unfortunately, this opens up a potential confusion for users, especially
if the are editing/creating a link embeddable in the security context.

- The Security app does not currently use real locators, and my attempts
to remedy this were unsuccessful. Therefore, rather than requiring a
**true** `Locator` object to be passed in as a prop to the
`DashboardRenderer`, I had to instead accept any object that has both a
`navigate` and `getRedirectUrl` method defined.

It would be **much** cleaner for the Security plugin to define its own
locator - that way, the `DashboardRenderer` could instead just accept a
locator ID as a prop, and the link embeddable could use that ID fetch
the appropriate locator as necessary; however, this is a pretty major
refactor, since the Security app handles URLs/navigation in a much
different way than any other Kibana app.


**Before:**


https://github.com/elastic/kibana/assets/8698078/67fdf34f-60e3-47fc-b205-8a6443a0452d

**After:**


https://github.com/elastic/kibana/assets/8698078/f92f1eb0-1467-4408-8792-f881e355188b

### APM @elastic/apm-ui 

While I did my best to modify each portable dashboard implementation to
include this new locator prop, without the full context on how each
plugin wants to handle the link embeddable, I've had to make a few
assumptions about the behaviour. For example, in APM....

- Similar to the Security implementation, the APM dashboards **not**
respond to the query bar in the same way that they do in the Dashboard
app - therefore, the link embeddable settings for "Use filters and query
from origin dashboard" and "Use date range from origin dashboard" do not
make sense in this context.

For example, in APM, it is (from my understanding) **expected** that the
query bar would always remain constant, regardless of these settings;
therefore, these settings are more-or-less ignored. That being said,
because APM does not currently support dashboard editing, I believe this
is less of a concern than it is for other implementations.

- Because of the unique content management system that APM is using,
where Dashboards must be linked in order for them to be viewed, the link
embeddable would not work **unless** the user was navigating to a
dashboard that was added to the APM Dashboard CM. I've had to change
this so that **any** dashboard can be viewed in the APM context - that
way, even if someone is trying to load a dashboard that is **not**
linked, it will still load.

This goes against how APM typically handles its dashboards, so I am open
to suggestions if this behaviour is undesirable. It just felt odd to me
that I would have to link every single referenced dashboard in the APM
context if I wanted the link embeddable to work....

**Before:**


https://github.com/elastic/kibana/assets/8698078/59397d42-2289-4721-9d4a-74c3e7a4d871

**After:**


https://github.com/elastic/kibana/assets/8698078/87924826-e766-4b50-87c6-132ae936f2fc


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Ruhshan pushed a commit to Ruhshan/kibana that referenced this issue Nov 7, 2023
Closes elastic#164748

## Summary

Previously, the link embeddable **always** used the Dashboard plugin's
locator - this meant that, for portable dashboards (such as those in
Security or in APM), navigation through the link embeddable would always
send the user to the Dashboard plugin rather than staying in whatever
context the portable dashboard was in. This PR fixes that by ensuring
that each `DashboardRenderer` consumer can provide their **own** locator
as a prop - this means that the Dashboard app can still use its own
locator, while the Security/APM plugins (for example) can **also**
define a locator so that navigation remains in the security/APM app.

### Security @elastic/security-solution-platform 

While I did my best to modify each portable dashboard implementation to
include this new locator prop, without the full context on how each
plugin wants to handle the link embeddable, I've had to make a few
assumptions about the behaviour. For example, in security....

- The Security dashboards **not** respond to the query bar in the same
way that they do in the Dashboard app - therefore, the link embeddable
settings for "Use filters and query from origin dashboard" and "Use date
range from origin dashboard" do not make sense in this context.

For example, in Security, it is (from my understanding) **expected**
that the query bar would always remain constant, regardless of these
settings; therefore, these settings are more-or-less ignored.
Unfortunately, this opens up a potential confusion for users, especially
if the are editing/creating a link embeddable in the security context.

- The Security app does not currently use real locators, and my attempts
to remedy this were unsuccessful. Therefore, rather than requiring a
**true** `Locator` object to be passed in as a prop to the
`DashboardRenderer`, I had to instead accept any object that has both a
`navigate` and `getRedirectUrl` method defined.

It would be **much** cleaner for the Security plugin to define its own
locator - that way, the `DashboardRenderer` could instead just accept a
locator ID as a prop, and the link embeddable could use that ID fetch
the appropriate locator as necessary; however, this is a pretty major
refactor, since the Security app handles URLs/navigation in a much
different way than any other Kibana app.


**Before:**


https://github.com/elastic/kibana/assets/8698078/67fdf34f-60e3-47fc-b205-8a6443a0452d

**After:**


https://github.com/elastic/kibana/assets/8698078/f92f1eb0-1467-4408-8792-f881e355188b

### APM @elastic/apm-ui 

While I did my best to modify each portable dashboard implementation to
include this new locator prop, without the full context on how each
plugin wants to handle the link embeddable, I've had to make a few
assumptions about the behaviour. For example, in APM....

- Similar to the Security implementation, the APM dashboards **not**
respond to the query bar in the same way that they do in the Dashboard
app - therefore, the link embeddable settings for "Use filters and query
from origin dashboard" and "Use date range from origin dashboard" do not
make sense in this context.

For example, in APM, it is (from my understanding) **expected** that the
query bar would always remain constant, regardless of these settings;
therefore, these settings are more-or-less ignored. That being said,
because APM does not currently support dashboard editing, I believe this
is less of a concern than it is for other implementations.

- Because of the unique content management system that APM is using,
where Dashboards must be linked in order for them to be viewed, the link
embeddable would not work **unless** the user was navigating to a
dashboard that was added to the APM Dashboard CM. I've had to change
this so that **any** dashboard can be viewed in the APM context - that
way, even if someone is trying to load a dashboard that is **not**
linked, it will still load.

This goes against how APM typically handles its dashboards, so I am open
to suggestions if this behaviour is undesirable. It just felt odd to me
that I would have to link every single referenced dashboard in the APM
context if I wanted the link embeddable to work....

**Before:**


https://github.com/elastic/kibana/assets/8698078/59397d42-2289-4721-9d4a-74c3e7a4d871

**After:**


https://github.com/elastic/kibana/assets/8698078/87924826-e766-4b50-87c6-132ae936f2fc


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:large Large Level of Effort Project:Dashboard Navigation Related to the Dashboard Navigation Project Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
No open projects
Dashboard
  
Inbox
Development

Successfully merging a pull request may close this issue.

3 participants