[Dashboard Navigation] Add functional + unit tests #161287
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
Heenawter
added
Feature:Dashboard
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
Heenawter
added
loe:medium
This was referenced Jul 5, 2023
Heenawter
added a commit
that referenced
this issue
Jul 10, 2023
Closes #154360 ## Summary This PR adds the first draft of the UI for creating the navigation embeddable and adding links to it. Note that this PR **only** addresses adding links - you cannot currently remove links from the panel or edit existing links. Also, because this PR contains critical code, some cleanup tasks were left as `TODO` in order to get it merged ASAP - these will be addressed in follow up PRs. https://github.com/elastic/kibana/assets/8698078/ac011207-1c4b-40cd-a151-78addb99d32d ### 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: kibanamachine <[email protected]>
7 tasks
Heenawter
added a commit
that referenced
this issue
Jul 17, 2023
Closes #154361 Closes #161274 Closes #161693 ## Summary This PR adds editing capabilities to the navigation embeddable, including deleting/editing existing links and reordering the list of links. It also fixes the delay in opening the editing flyout from the async import in `getExplicitInput` (from the navigation embeddable factory) by moving it to the constructor of the factory. https://github.com/elastic/kibana/assets/8698078/ace9dcd4-0607-40de-959e-94348a5fa4fa ### 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: kibanamachine <[email protected]>
7 tasks
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 <[email protected]> Co-authored-by: kibanamachine <[email protected]>
7 tasks
nickpeihl
added a commit
that referenced
this issue
Sep 5, 2023
Closes #154381 ## Summary This PR adds functionality to the link embeddable so that 1) dashboard to dashboard links will now navigate between dashboards, taking the context (filters, time range, queries, etc.) as the user navigates (assuming the appropriate settings are applied) and 2) URL links will now navigate to internal and/or external URLs as expected ### Dashboard Links https://github.com/elastic/kibana/assets/8698078/1034b454-3add-48c2-8505-44d89e2d87d0 > **Note** > In the above video, all links were configured to include queries, filter pills, and the selected time range in the passed context. It is possible to disable these settings, if necessary. #### 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 - When testing, special attention should be paid to the different link settings - ensure that all settings are respected. - Ensure that `ctrl + click`, `shift + click`, and `meta + click` work as expected, regardless of whether the `Open 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 https://github.com/elastic/kibana/assets/8698078/5abf9772-17b3-4ab4-a704-4a3ff432fa23 #### 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: 1) links that do not fit the expected format; for example, we do not accept any links that start with something other than `http`, `https`, or `mailto`. 2) links that are disallowed due to the `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: ``` externalUrl.policy: - allow: false host: danger.example.com - allow: true host: example.com protocol: https ``` 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`, and `meta + click` work as expected, regardless of whether the `Open in new tab` setting is true or false. ### 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) > **Note** > How Chrome and Firefox handle modified `onClick` events varies significantly and, through this testing, I found a bug where `shift + click`ing was previously not opening the links in a new tab in Firefox specifically - this is why I had to use the default to `href` behaviour whenever possible. Something to keep in mind when testing. ### 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 <[email protected]> Co-authored-by: Nick Peihl <[email protected]> Co-authored-by: Nick Peihl <[email protected]>
6 tasks
nickpeihl
added a commit
that referenced
this issue
Sep 20, 2023
Part of #161287 These unit tests were designed to test complex cases for the components. Please review carefully and suggest any test cases that I may have overlooked.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Before the navigation embeddable feature branch can be merged into the main branch, we should add both functional and unit tests (focusing primarily on unit tests whenever possible) to cover all basic functionality.
The text was updated successfully, but these errors were encountered: