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] Better error handling for deleted dashboards #161563

Closed
Tracked by #154354
nickpeihl opened this issue Jul 10, 2023 · 2 comments
Closed
Tracked by #154354

[Dashboard Navigation] Better error handling for deleted dashboards #161563

nickpeihl opened this issue Jul 10, 2023 · 2 comments
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:medium Medium Level of Effort Project:Dashboard Navigation Related to the Dashboard Navigation Project Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas

Comments

@nickpeihl
Copy link
Member

If a Links panel includes a reference to a deleted Dashboard we only return an error in console and an empty link in the panel. We should return a user-facing error.

Screenshot 2023-07-10 at 11 12 09 AM
@nickpeihl nickpeihl 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 Jul 10, 2023
@elasticmachine
Copy link
Contributor

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

@Heenawter Heenawter added the Feature:Dashboard Dashboard related features label Jul 11, 2023
@Heenawter Heenawter self-assigned this Jul 18, 2023
Heenawter added a commit that referenced this issue Aug 16, 2023
… error handling (#162285)

Closes #154357
Closes #161563

## Summary

> **Warning**
> I will be waiting to merge this PR until **after**
#160896 is merged - I am simply
opening it early so that we can start the design review process 👍

### Layout 
This PR improves the rendering of the navigation embeddable to include
both a horizontal and vertical layout option, as well as changing the
style of how the links are rendered:
    

https://github.com/elastic/kibana/assets/8698078/37d27683-a6c4-4e7a-9589-0eb0fb899e98
    
A known issue with the horizontal layout is that, as demonstrated in the
above video, a "compact" horizontal navigation panel does not render as
nicely in edit mode versus view mode - this is an **overall panel
problem** and not specifically a problem with the navigation embeddable
(although the navigation embeddable definitely makes it more obvious).
This will be resolved for **all panels** by [removing the panel header
altogether](#162182).
 
### Error handling
This PR adds proper error handling to the navigation embeddable so that,
if a dashboard link is "broken" (i.e. the destination dashboard has been
deleted or cannot be fetched), an appropriate error message shows up in
both the component and the editor flyout:


https://github.com/elastic/kibana/assets/8698078/33a3e573-36a2-47ca-b367-3e04f9541ca3

> **Note**
> When possible, we want to provide the user with as much context as
possible for broken dashboard links - that is why, if a dashboard link
was given a custom label, we still show this custom label even when the
destination dashboard has been deleted/is unreachable.
>
> However, once a dashboard has been deleted, we no longer know what the
title of that dashboard was because the saved object no longer exists -
so, if a dashboard link is **not** given a custom label and the
destination dashboard is deleted, we default to the "Error fetching
dashboard" error message instead. In order to create a distinction
between these two scenarios (a broken dashboard link with a custom label
versus without), we italicize the generic "Error fetching dashboard"
error text.

### Improved efficiency
Previously, the navigation embeddable was handling its **own** dashboard
cache, which meant that (a) every single embeddable had its own cache
and (b) the navigation embeddable code had to be mindful when choosing
to use the memoized/cached version of the dashboard versus fetching it
fresh.

After discussing with @ThomThomson about how to better handle this, we
opted to move this logic to the dashboard content management service -
not only does this clean up the navigation embeddable code, it also
improves all the loading of dashboards in general. For example, consider
the following video where I was testing re-loading a previously loaded
dashboard on a throttled `Slow 3G` network:


https://github.com/elastic/kibana/assets/8698078/41d68ac7-557c-4586-a59b-7268086991dd

Notice in the above video how much faster the secondary load of the
dashboard is in comparison to the first initial load - this is because,
in the second load, we can hit the cache instead of re-fetching the
dashboard from the content management client, which allows us to skip an
entire loading state.


### Checklist

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ] ~[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~ Will be
addressed in #161287
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### 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: Andrea Del Rio <delrio.andre@gmail.com>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
@Heenawter
Copy link
Contributor

Closed by #162285

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:medium Medium Level of Effort Project:Dashboard Navigation Related to the Dashboard Navigation Project Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
None yet
Development

No branches or pull requests

3 participants