[Dashboard Navigation] Add horizontal/vertical embeddable rendering + error handling #162285
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Heenawter
added
Team:Presentation
Heenawter
force-pushed
the
embeddable-rendering-ui_2023-07-18
branch
from
d58c6c1
to
86ac78f
Compare
Heenawter
force-pushed
the
embeddable-rendering-ui_2023-07-18
branch
from
86ac78f
to
09091ea
Compare
Heenawter
commented
Jul 25, 2023
...plugins/navigation_embeddable/public/components/editor/navigation_embeddable_link_editor.tsx
Show resolved
Hide resolved
Heenawter
commented
Jul 25, 2023
...lugins/navigation_embeddable/public/components/editor/navigation_embeddable_panel_editor.tsx
Outdated
Show resolved
Hide resolved
Heenawter
commented
Jul 25, 2023
src/plugins/navigation_embeddable/public/components/navigation_embeddable_component.scss
Outdated
Show resolved
Hide resolved
Heenawter
commented
Jul 25, 2023
src/plugins/navigation_embeddable/public/components/navigation_embeddable_component.scss
Outdated
Show resolved
Hide resolved
Heenawter
commented
Jul 25, 2023
src/plugins/navigation_embeddable/public/components/navigation_embeddable_component.scss
Outdated
Show resolved
Hide resolved
Heenawter
commented
Jul 25, 2023
src/plugins/navigation_embeddable/public/components/navigation_embeddable_component.tsx
Show resolved
Hide resolved
Heenawter
commented
Jul 25, 2023
Heenawter
force-pushed
the
embeddable-rendering-ui_2023-07-18
branch
from
8b4e240
to
1011d65
Compare
|
@nickpeihl @andreadelrio I have updated this PR to replace the dual save buttons with the "Save to library" Screen.Recording.2023-08-15.at.12.35.02.PM.mov |
…-ref HEAD~1..HEAD --fix'
nickpeihl
approved these changes
Aug 15, 2023
|
This is looking great! At this point, I would like to get @amyjtechwriter to help us with the copy. Should we capitalize 
@nickpeihl @Heenawter I think this modal works, but I think adding "to library" at the end of the heading will add clarity as to why this modal shows up when users have the "save to library" toggle set to true and hit Some more context: When we show the menu for a Lens visualization we have an item that read "Edit Lens" because Lens is a proper noun. Should we give links panels the same treatment even though Links is not really a separate app? 
|
For reference, here are some other save modals for different embeddable types:
Based on the above, I would agree that changing it to
I think the capitalization of the We can ensure that it matches for the links embeddable in this feature branch (in this case, I think that lowercase If we agree, I can definitely file a separate issue 👍
For consistency, we would probably want to add Similar to above, I think it might be worth fixing this inconstancy in main as part of the dashboard usability initiative so that both (1) we don't muddy up the link embeddable feature branch with non-critical / tangentially related work and (2) we don't have to wait for the links embeddable to be available on cc @cqliu1 for the dashboard usability mentions 👀 |
andreadelrio
approved these changes
Aug 16, 2023
There was a problem hiding this comment.
Design changes LGTM 🎉. This is coming along very nicely, great job @Heenawter and @nickpeihl! Agreed that we can take care of copy consistency issues on main rather than on this branch 👍.
|
Hello hello @Heenawter and @andreadelrio, this might be an unnecessary opinion here if the copy issues are going to be addressed on a different branch, but I really like your combined solutions of |
afharo
approved these changes
Aug 16, 2023
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @Heenawter |
This was referenced Aug 16, 2023
Heenawter
added a commit
that referenced
this pull request
Aug 16, 2023
## Summary As part of a [larger navigation embeddable discussion](#162285 (comment)), it was noted that there were inconsistencies between how we display the panel type between the saved object modal and the context menu action. Turns out, this is because we were using the embeddable `type` in the `"Save to library"` modal title while the context menu action uses the factory's `displayName` - while this coincidentally worked **in most cases** (`maps` and `visualization`), it **does not** work for Lens panels, since we want to use the proper noun `"Lens"` in this case rather than the saved object type `"lens"`: <p align="center"><img width="500" src="https://github.com/elastic/kibana/assets/8698078/d56659bf-ff13-4974-80bb-f267fdcb1cff"/></p> This was even **more** obvious with the navigation embeddable, since the saved object type is currently `"navigation_embeddable"` (this may change in the future): <p align="center"><img width="500" src="https://github.com/elastic/kibana/assets/8698078/868daebb-fa51-4f97-bd23-3cf47472fadf"/></p> Therefore, by switching to use the factory's `displayName` (if available), the saved object modal will now stay consistent with the `"Edit <x>"` option in the context menu for each panel. | Before | After | |--------|--------| |  |  | |  | | |  | <br><p align="center">**No change**, as expected.</p> | |  | <br><p align="center">**No change**, as expected.</p> | Note that, in the above "After" screenshots, `Links` is still capitalized unnecessarily - this is because, if we change the factory's `displayName` to be the lowercase `"links"`, this fixes the modal but it **also** impacts the item in the add panel context menu and the panel actions context menu, like so: | Add panel context menu | Panel actions context menu | |--------|--------| |  | | This is because the link embeddable is not currently registered as a "visualization" and so the `"Add panel"` context menu logic is handled as part of `getEmbeddableFactoryMenuItem` in `./src/plugins/dashboard/public/dashboard_app/top_nav/editor_menu.tsx` rather than `getVisTypeMenuItem` (this **could** potentially be fixed by #162840 since we have custom logic for displaying aliases). While the lowercase `"links"` in the panel actions context menu is **desired**, it is **not** desirable to have the lowercase `"links"` in the add panel context menu - however, it is not currently possible to change one without changing the other. Note that this is **also** true for the `"Image"` embeddable - by changing the `displayName` of the image factory from `"Image"` to `"image"`, the `"Edit"` panel actions context menu item would be displayed correctly as `"Edit image"` (currently, it is `"Edit Image"`) but the lowercase `"image"` would also be displayed in the add panel context menu, which is undesirable. It will require a larger refactor to fix these inconsistencies, so it should be addressed separately. ### 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) ### 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)
3 tasks
Heenawter
added a commit
that referenced
this pull request
Jan 26, 2024
## Summary This PR fixes the alias redirect problem that was noted in the attached SDH. For context, as part of the [Links panel work](#166896), I added [dashboard caching](#162285) to make navigation more efficient - however, when implementing this, I did not consider the redirect behaviour. Specifically, we were **always** adding dashboards to the cache, even if the load outcome meant a redirect was necessary - so, because the meta info for the dashboard fetch result was cached, this meant that the `'aliasMatch'` outcome was **also** cached. Because of this, after a redirect happened, the second attempt to load the dashboard would return the result from the **cache** rather than fetching the dashboard from the CM client - but, as described previously, the cached dashboard would still appear as though it required a redirect because the cached outcome was `'aliasMatch'`. Therefore, because of hitting this early return on **both** fetch attempts (before the redirect, after the redirect)... https://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171 ... the dashboard information would not get updated. _(Oof!)_ Despite the lengthy description of the problem, the solution is quite simple - just don't add dashboards to the cache if they require a redirect. And then everything works! 🎉 ### Videos **Before** https://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f **After** https://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1 ### Testing To test, I followed the directions from [this PR description](#163658) to get a dashboard that requires a redirect - then, I created a markdown panel with the **old** (from the Default space) dashboard ID in the new space to see the same redirect behaviour that the customer in the SDH was seeing. ### Checklist - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) - [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)
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Jan 26, 2024
## Summary This PR fixes the alias redirect problem that was noted in the attached SDH. For context, as part of the [Links panel work](elastic#166896), I added [dashboard caching](elastic#162285) to make navigation more efficient - however, when implementing this, I did not consider the redirect behaviour. Specifically, we were **always** adding dashboards to the cache, even if the load outcome meant a redirect was necessary - so, because the meta info for the dashboard fetch result was cached, this meant that the `'aliasMatch'` outcome was **also** cached. Because of this, after a redirect happened, the second attempt to load the dashboard would return the result from the **cache** rather than fetching the dashboard from the CM client - but, as described previously, the cached dashboard would still appear as though it required a redirect because the cached outcome was `'aliasMatch'`. Therefore, because of hitting this early return on **both** fetch attempts (before the redirect, after the redirect)... https://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171 ... the dashboard information would not get updated. _(Oof!)_ Despite the lengthy description of the problem, the solution is quite simple - just don't add dashboards to the cache if they require a redirect. And then everything works! 🎉 ### Videos **Before** https://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f **After** https://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1 ### Testing To test, I followed the directions from [this PR description](elastic#163658) to get a dashboard that requires a redirect - then, I created a markdown panel with the **old** (from the Default space) dashboard ID in the new space to see the same redirect behaviour that the customer in the SDH was seeing. ### Checklist - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) - [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) (cherry picked from commit 27b34d5)
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Jan 26, 2024
## Summary This PR fixes the alias redirect problem that was noted in the attached SDH. For context, as part of the [Links panel work](elastic#166896), I added [dashboard caching](elastic#162285) to make navigation more efficient - however, when implementing this, I did not consider the redirect behaviour. Specifically, we were **always** adding dashboards to the cache, even if the load outcome meant a redirect was necessary - so, because the meta info for the dashboard fetch result was cached, this meant that the `'aliasMatch'` outcome was **also** cached. Because of this, after a redirect happened, the second attempt to load the dashboard would return the result from the **cache** rather than fetching the dashboard from the CM client - but, as described previously, the cached dashboard would still appear as though it required a redirect because the cached outcome was `'aliasMatch'`. Therefore, because of hitting this early return on **both** fetch attempts (before the redirect, after the redirect)... https://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171 ... the dashboard information would not get updated. _(Oof!)_ Despite the lengthy description of the problem, the solution is quite simple - just don't add dashboards to the cache if they require a redirect. And then everything works! 🎉 ### Videos **Before** https://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f **After** https://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1 ### Testing To test, I followed the directions from [this PR description](elastic#163658) to get a dashboard that requires a redirect - then, I created a markdown panel with the **old** (from the Default space) dashboard ID in the new space to see the same redirect behaviour that the customer in the SDH was seeing. ### Checklist - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) - [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) (cherry picked from commit 27b34d5)
kibanamachine
added a commit
that referenced
this pull request
Jan 26, 2024
# Backport This will backport the following commits from `main` to `8.12`: - [[Dashboard] Only cache non-alias dashboards (#175635)](#175635) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Hannah Mudge","email":"Heenawter@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-01-26T16:09:28Z","message":"[Dashboard] Only cache non-alias dashboards (#175635)\n\n## Summary\r\n\r\nThis PR fixes the alias redirect problem that was noted in the attached\r\nSDH.\r\n\r\nFor context, as part of the [Links panel\r\nwork](#166896), I added [dashboard\r\ncaching](#162285) to make\r\nnavigation more efficient - however, when implementing this, I did not\r\nconsider the redirect behaviour.\r\n\r\nSpecifically, we were **always** adding dashboards to the cache, even if\r\nthe load outcome meant a redirect was necessary - so, because the meta\r\ninfo for the dashboard fetch result was cached, this meant that the\r\n`'aliasMatch'` outcome was **also** cached. Because of this, after a\r\nredirect happened, the second attempt to load the dashboard would return\r\nthe result from the **cache** rather than fetching the dashboard from\r\nthe CM client - but, as described previously, the cached dashboard would\r\nstill appear as though it required a redirect because the cached outcome\r\nwas `'aliasMatch'`.\r\n\r\nTherefore, because of hitting this early return on **both** fetch\r\nattempts (before the redirect, after the redirect)...\r\n\r\n\r\nhttps://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171\r\n\r\n... the dashboard information would not get updated. _(Oof!)_ \r\n\r\nDespite the lengthy description of the problem, the solution is quite\r\nsimple - just don't add dashboards to the cache if they require a\r\nredirect. And then everything works! 🎉\r\n\r\n### Videos\r\n**Before**\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f\r\n\r\n\r\n**After**\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1\r\n\r\n\r\n### Testing\r\nTo test, I followed the directions from [this PR\r\ndescription](#163658) to get a\r\ndashboard that requires a redirect - then, I created a markdown panel\r\nwith the **old** (from the Default space) dashboard ID in the new space\r\nto see the same redirect behaviour that the customer in the SDH was\r\nseeing.\r\n\r\n### Checklist\r\n- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"27b34d591111a878001eaac7b0f3ef0173bc8f02","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Feature:Dashboard","release_note:fix","Team:Presentation","loe:small","impact:high","backport:prev-minor","v8.12.1","v8.13.0"],"title":"[Dashboard] Only cache non-alias dashboards","number":175635,"url":"https://github.com/elastic/kibana/pull/175635","mergeCommit":{"message":"[Dashboard] Only cache non-alias dashboards (#175635)\n\n## Summary\r\n\r\nThis PR fixes the alias redirect problem that was noted in the attached\r\nSDH.\r\n\r\nFor context, as part of the [Links panel\r\nwork](#166896), I added [dashboard\r\ncaching](#162285) to make\r\nnavigation more efficient - however, when implementing this, I did not\r\nconsider the redirect behaviour.\r\n\r\nSpecifically, we were **always** adding dashboards to the cache, even if\r\nthe load outcome meant a redirect was necessary - so, because the meta\r\ninfo for the dashboard fetch result was cached, this meant that the\r\n`'aliasMatch'` outcome was **also** cached. Because of this, after a\r\nredirect happened, the second attempt to load the dashboard would return\r\nthe result from the **cache** rather than fetching the dashboard from\r\nthe CM client - but, as described previously, the cached dashboard would\r\nstill appear as though it required a redirect because the cached outcome\r\nwas `'aliasMatch'`.\r\n\r\nTherefore, because of hitting this early return on **both** fetch\r\nattempts (before the redirect, after the redirect)...\r\n\r\n\r\nhttps://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171\r\n\r\n... the dashboard information would not get updated. _(Oof!)_ \r\n\r\nDespite the lengthy description of the problem, the solution is quite\r\nsimple - just don't add dashboards to the cache if they require a\r\nredirect. And then everything works! 🎉\r\n\r\n### Videos\r\n**Before**\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f\r\n\r\n\r\n**After**\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1\r\n\r\n\r\n### Testing\r\nTo test, I followed the directions from [this PR\r\ndescription](#163658) to get a\r\ndashboard that requires a redirect - then, I created a markdown panel\r\nwith the **old** (from the Default space) dashboard ID in the new space\r\nto see the same redirect behaviour that the customer in the SDH was\r\nseeing.\r\n\r\n### Checklist\r\n- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"27b34d591111a878001eaac7b0f3ef0173bc8f02"}},"sourceBranch":"main","suggestedTargetBranches":["8.12"],"targetPullRequestStates":[{"branch":"8.12","label":"v8.12.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.13.0","branchLabelMappingKey":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/175635","number":175635,"mergeCommit":{"message":"[Dashboard] Only cache non-alias dashboards (#175635)\n\n## Summary\r\n\r\nThis PR fixes the alias redirect problem that was noted in the attached\r\nSDH.\r\n\r\nFor context, as part of the [Links panel\r\nwork](#166896), I added [dashboard\r\ncaching](#162285) to make\r\nnavigation more efficient - however, when implementing this, I did not\r\nconsider the redirect behaviour.\r\n\r\nSpecifically, we were **always** adding dashboards to the cache, even if\r\nthe load outcome meant a redirect was necessary - so, because the meta\r\ninfo for the dashboard fetch result was cached, this meant that the\r\n`'aliasMatch'` outcome was **also** cached. Because of this, after a\r\nredirect happened, the second attempt to load the dashboard would return\r\nthe result from the **cache** rather than fetching the dashboard from\r\nthe CM client - but, as described previously, the cached dashboard would\r\nstill appear as though it required a redirect because the cached outcome\r\nwas `'aliasMatch'`.\r\n\r\nTherefore, because of hitting this early return on **both** fetch\r\nattempts (before the redirect, after the redirect)...\r\n\r\n\r\nhttps://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171\r\n\r\n... the dashboard information would not get updated. _(Oof!)_ \r\n\r\nDespite the lengthy description of the problem, the solution is quite\r\nsimple - just don't add dashboards to the cache if they require a\r\nredirect. And then everything works! 🎉\r\n\r\n### Videos\r\n**Before**\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f\r\n\r\n\r\n**After**\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1\r\n\r\n\r\n### Testing\r\nTo test, I followed the directions from [this PR\r\ndescription](#163658) to get a\r\ndashboard that requires a redirect - then, I created a markdown panel\r\nwith the **old** (from the Default space) dashboard ID in the new space\r\nto see the same redirect behaviour that the customer in the SDH was\r\nseeing.\r\n\r\n### Checklist\r\n- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"27b34d591111a878001eaac7b0f3ef0173bc8f02"}}]}] BACKPORT--> Co-authored-by: Hannah Mudge <Heenawter@users.noreply.github.com>
lcawl
pushed a commit
to lcawl/kibana
that referenced
this pull request
Jan 26, 2024
## Summary This PR fixes the alias redirect problem that was noted in the attached SDH. For context, as part of the [Links panel work](elastic#166896), I added [dashboard caching](elastic#162285) to make navigation more efficient - however, when implementing this, I did not consider the redirect behaviour. Specifically, we were **always** adding dashboards to the cache, even if the load outcome meant a redirect was necessary - so, because the meta info for the dashboard fetch result was cached, this meant that the `'aliasMatch'` outcome was **also** cached. Because of this, after a redirect happened, the second attempt to load the dashboard would return the result from the **cache** rather than fetching the dashboard from the CM client - but, as described previously, the cached dashboard would still appear as though it required a redirect because the cached outcome was `'aliasMatch'`. Therefore, because of hitting this early return on **both** fetch attempts (before the redirect, after the redirect)... https://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171 ... the dashboard information would not get updated. _(Oof!)_ Despite the lengthy description of the problem, the solution is quite simple - just don't add dashboards to the cache if they require a redirect. And then everything works! 🎉 ### Videos **Before** https://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f **After** https://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1 ### Testing To test, I followed the directions from [this PR description](elastic#163658) to get a dashboard that requires a redirect - then, I created a markdown panel with the **old** (from the Default space) dashboard ID in the new space to see the same redirect behaviour that the customer in the SDH was seeing. ### Checklist - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) - [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)
CoenWarmer
pushed a commit
to CoenWarmer/kibana
that referenced
this pull request
Feb 15, 2024
## Summary This PR fixes the alias redirect problem that was noted in the attached SDH. For context, as part of the [Links panel work](elastic#166896), I added [dashboard caching](elastic#162285) to make navigation more efficient - however, when implementing this, I did not consider the redirect behaviour. Specifically, we were **always** adding dashboards to the cache, even if the load outcome meant a redirect was necessary - so, because the meta info for the dashboard fetch result was cached, this meant that the `'aliasMatch'` outcome was **also** cached. Because of this, after a redirect happened, the second attempt to load the dashboard would return the result from the **cache** rather than fetching the dashboard from the CM client - but, as described previously, the cached dashboard would still appear as though it required a redirect because the cached outcome was `'aliasMatch'`. Therefore, because of hitting this early return on **both** fetch attempts (before the redirect, after the redirect)... https://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171 ... the dashboard information would not get updated. _(Oof!)_ Despite the lengthy description of the problem, the solution is quite simple - just don't add dashboards to the cache if they require a redirect. And then everything works! 🎉 ### Videos **Before** https://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f **After** https://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1 ### Testing To test, I followed the directions from [this PR description](elastic#163658) to get a dashboard that requires a redirect - then, I created a markdown panel with the **old** (from the Default space) dashboard ID in the new space to see the same redirect behaviour that the customer in the SDH was seeing. ### Checklist - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) - [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)
CoenWarmer
pushed a commit
to CoenWarmer/kibana
that referenced
this pull request
Feb 15, 2024
## Summary This PR fixes the alias redirect problem that was noted in the attached SDH. For context, as part of the [Links panel work](elastic#166896), I added [dashboard caching](elastic#162285) to make navigation more efficient - however, when implementing this, I did not consider the redirect behaviour. Specifically, we were **always** adding dashboards to the cache, even if the load outcome meant a redirect was necessary - so, because the meta info for the dashboard fetch result was cached, this meant that the `'aliasMatch'` outcome was **also** cached. Because of this, after a redirect happened, the second attempt to load the dashboard would return the result from the **cache** rather than fetching the dashboard from the CM client - but, as described previously, the cached dashboard would still appear as though it required a redirect because the cached outcome was `'aliasMatch'`. Therefore, because of hitting this early return on **both** fetch attempts (before the redirect, after the redirect)... https://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171 ... the dashboard information would not get updated. _(Oof!)_ Despite the lengthy description of the problem, the solution is quite simple - just don't add dashboards to the cache if they require a redirect. And then everything works! 🎉 ### Videos **Before** https://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f **After** https://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1 ### Testing To test, I followed the directions from [this PR description](elastic#163658) to get a dashboard that requires a redirect - then, I created a markdown panel with the **old** (from the Default space) dashboard ID in the new space to see the same redirect behaviour that the customer in the SDH was seeing. ### Checklist - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) - [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)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #154357
Closes #161563
Summary
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:
Screen.Recording.2023-07-25.at.11.23.49.AM.mov
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.
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:
Screen.Recording.2023-07-25.at.12.14.28.PM.mov
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 3Gnetwork:Screen.Recording.2023-07-20.at.3.10.49.PM.mov
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
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