Skip to content

Commit

Permalink
[Dashboard Navigation] Add horizontal/vertical embeddable rendering +…
Browse files Browse the repository at this point in the history
… 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]>
  • Loading branch information
3 people authored Aug 16, 2023
1 parent 577134c commit f0ebcb2
Show file tree
Hide file tree
Showing 38 changed files with 802 additions and 344 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1055,6 +1055,7 @@
}
},
"navigation_embeddable": {
"dynamic": false,
"properties": {
"id": {
"type": "text"
Expand Down
6 changes: 6 additions & 0 deletions src/plugins/dashboard/public/dashboard_constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,14 @@ export const DEFAULT_PANEL_WIDTH = DASHBOARD_GRID_COLUMN_COUNT / 2;

export const CHANGE_CHECK_DEBOUNCE = 100;

// ------------------------------------------------------------------
// Content Management
// ------------------------------------------------------------------
export { CONTENT_ID as DASHBOARD_CONTENT_ID } from '../common/content_management/constants';

export const DASHBOARD_CACHE_SIZE = 20; // only store a max of 20 dashboards
export const DASHBOARD_CACHE_TTL = 1000 * 60 * 5; // time to live = 5 minutes

// ------------------------------------------------------------------
// Default State
// ------------------------------------------------------------------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export const dashboardContentManagementServiceFactory: DashboardContentManagemen
hits,
});
}),
findById: jest.fn(),
findByIds: jest.fn().mockImplementation(() =>
Promise.resolve([
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import LRUCache from 'lru-cache';
import { DashboardCrudTypes } from '../../../common/content_management';
import { DASHBOARD_CACHE_SIZE, DASHBOARD_CACHE_TTL } from '../../dashboard_constants';

export class DashboardContentManagementCache {
private cache: LRUCache<string, DashboardCrudTypes['GetOut']>;

constructor() {
this.cache = new LRUCache<string, DashboardCrudTypes['GetOut']>({
max: DASHBOARD_CACHE_SIZE,
maxAge: DASHBOARD_CACHE_TTL,
});
}

/** Fetch the dashboard with `id` from the cache */
public fetchDashboard(id: string) {
return this.cache.get(id);
}

/** Add the fetched dashboard to the cache */
public addDashboard({ item: dashboard, meta }: DashboardCrudTypes['GetOut']) {
this.cache.set(dashboard.id, {
meta,
item: dashboard,
});
}

/** Delete the dashboard with `id` from the cache */
public deleteDashboard(id: string) {
this.cache.del(id);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { checkForDuplicateDashboardTitle } from './lib/check_for_duplicate_dashb

import {
searchDashboards,
findDashboardById,
findDashboardsByIds,
findDashboardIdByTitle,
} from './lib/find_dashboards';
Expand All @@ -23,13 +24,16 @@ import type {
} from './types';
import { loadDashboardState } from './lib/load_dashboard_state';
import { deleteDashboards } from './lib/delete_dashboards';
import { DashboardContentManagementCache } from './dashboard_content_management_cache';

export type DashboardContentManagementServiceFactory = KibanaPluginServiceFactory<
DashboardContentManagementService,
DashboardStartDependencies,
DashboardContentManagementRequiredServices
>;

export const dashboardContentManagementCache = new DashboardContentManagementCache();

export const dashboardContentManagementServiceFactory: DashboardContentManagementServiceFactory = (
{ startPlugins: { contentManagement } },
requiredServices
Expand Down Expand Up @@ -74,6 +78,7 @@ export const dashboardContentManagementServiceFactory: DashboardContentManagemen
search,
size,
}),
findById: (id) => findDashboardById(contentManagement, id),
findByIds: (ids) => findDashboardsByIds(contentManagement, ids),
findByTitle: (title) => findDashboardIdByTitle(contentManagement, title),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,22 @@
import { DashboardStartDependencies } from '../../../plugin';
import { DASHBOARD_CONTENT_ID } from '../../../dashboard_constants';
import { DashboardCrudTypes } from '../../../../common/content_management';
import { dashboardContentManagementCache } from '../dashboard_content_management_service';

export const deleteDashboards = async (
ids: string[],
contentManagement: DashboardStartDependencies['contentManagement']
) => {
const deletePromises = ids.map((id) =>
contentManagement.client.delete<
const deletePromises = ids.map((id) => {
dashboardContentManagementCache.deleteDashboard(id);
return contentManagement.client.delete<
DashboardCrudTypes['DeleteIn'],
DashboardCrudTypes['DeleteOut']
>({
contentTypeId: DASHBOARD_CONTENT_ID,
id,
})
);
});
});

await Promise.all(deletePromises);
};
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
} from '../../../../common/content_management';
import { DashboardStartDependencies } from '../../../plugin';
import { DASHBOARD_CONTENT_ID } from '../../../dashboard_constants';
import { dashboardContentManagementCache } from '../dashboard_content_management_service';

export interface SearchDashboardsArgs {
contentManagement: DashboardStartDependencies['contentManagement'];
Expand Down Expand Up @@ -67,23 +68,41 @@ export type FindDashboardsByIdResponse = { id: string } & (
| { status: 'error'; error: SavedObjectError }
);

export async function findDashboardsByIds(
export async function findDashboardById(
contentManagement: DashboardStartDependencies['contentManagement'],
ids: string[]
): Promise<FindDashboardsByIdResponse[]> {
const findPromises = ids.map((id) =>
contentManagement.client.get<DashboardCrudTypes['GetIn'], DashboardCrudTypes['GetOut']>({
id: string
): Promise<FindDashboardsByIdResponse> {
/** If the dashboard exists in the cache, then return the result from that */
const cachedDashboard = dashboardContentManagementCache.fetchDashboard(id);
if (cachedDashboard) {
return {
id,
status: 'success',
attributes: cachedDashboard.item.attributes,
};
}
/** Otherwise, fetch the dashboard from the content management client, add it to the cache, and return the result */
const response = await contentManagement.client
.get<DashboardCrudTypes['GetIn'], DashboardCrudTypes['GetOut']>({
contentTypeId: DASHBOARD_CONTENT_ID,
id,
})
);
const results = await Promise.all(findPromises);
.then((result) => {
dashboardContentManagementCache.addDashboard(result);
return { id, status: 'success', attributes: result.item.attributes };
})
.catch((e) => ({ status: 'error', error: e.body, id }));

return results.map((result) => {
if (result.item.error) return { status: 'error', error: result.item.error, id: result.item.id };
const { attributes, id } = result.item;
return { id, status: 'success', attributes };
});
return response as FindDashboardsByIdResponse;
}

export async function findDashboardsByIds(
contentManagement: DashboardStartDependencies['contentManagement'],
ids: string[]
): Promise<FindDashboardsByIdResponse[]> {
const findPromises = ids.map((id) => findDashboardById(contentManagement, id));
const results = await Promise.all(findPromises);
return results as FindDashboardsByIdResponse[];
}

export async function findDashboardIdByTitle(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
import { DashboardCrudTypes } from '../../../../common/content_management';
import type { LoadDashboardFromSavedObjectProps, LoadDashboardReturn } from '../types';
import { DASHBOARD_CONTENT_ID, DEFAULT_DASHBOARD_INPUT } from '../../../dashboard_constants';
import { dashboardContentManagementCache } from '../dashboard_content_management_service';

export function migrateLegacyQuery(query: Query | { [key: string]: any } | string): Query {
// Lucene was the only option before, so language-less queries are all lucene
Expand Down Expand Up @@ -58,14 +59,28 @@ export const loadDashboardState = async ({
/**
* Load the saved object from Content Management
*/
const { item: rawDashboardContent, meta: resolveMeta } = await contentManagement.client
.get<DashboardCrudTypes['GetIn'], DashboardCrudTypes['GetOut']>({
contentTypeId: DASHBOARD_CONTENT_ID,
id,
})
.catch((e) => {
throw new SavedObjectNotFound(DASHBOARD_CONTENT_ID, id);
});
let rawDashboardContent;
let resolveMeta;

const cachedDashboard = dashboardContentManagementCache.fetchDashboard(id);
if (cachedDashboard) {
/** If the dashboard exists in the cache, use the cached version to load the dashboard */
({ item: rawDashboardContent, meta: resolveMeta } = cachedDashboard);
} else {
/** Otherwise, fetch and load the dashboard from the content management client, and add it to the cache */
const result = await contentManagement.client
.get<DashboardCrudTypes['GetIn'], DashboardCrudTypes['GetOut']>({
contentTypeId: DASHBOARD_CONTENT_ID,
id,
})
.catch((e) => {
throw new SavedObjectNotFound(DASHBOARD_CONTENT_ID, id);
});

dashboardContentManagementCache.addDashboard(result);
({ item: rawDashboardContent, meta: resolveMeta } = result);
}

if (!rawDashboardContent || !rawDashboardContent.version) {
return {
dashboardInput: newDashboardState,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import { DashboardStartDependencies } from '../../../plugin';
import { DASHBOARD_CONTENT_ID } from '../../../dashboard_constants';
import { DashboardCrudTypes, DashboardAttributes } from '../../../../common/content_management';
import { dashboardSaveToastStrings } from '../../../dashboard_container/_dashboard_container_strings';
import { dashboardContentManagementCache } from '../dashboard_content_management_service';

export const serializeControlGroupInput = (
controlGroupInput: DashboardContainerInput['controlGroupInput']
Expand Down Expand Up @@ -200,6 +201,8 @@ export const saveDashboardState = async ({
if (newId !== lastSavedId) {
dashboardSessionStorage.clearState(lastSavedId);
return { redirectRequired: true, id: newId };
} else {
dashboardContentManagementCache.deleteDashboard(newId); // something changed in an existing dashboard, so delete it from the cache so that it can be re-fetched
}
}
return { id: newId };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ export interface FindDashboardsService {
'hasReference' | 'hasNoReference' | 'search' | 'size' | 'options'
>
) => Promise<SearchDashboardsResponse>;
findById: (id: string) => Promise<FindDashboardsByIdResponse>;
findByIds: (ids: string[]) => Promise<FindDashboardsByIdResponse[]>;
findByTitle: (title: string) => Promise<{ id: string } | undefined>;
}
7 changes: 7 additions & 0 deletions src/plugins/navigation_embeddable/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,10 @@ export const APP_ICON = 'link';
export const APP_NAME = i18n.translate('navigationEmbeddable.visTypeAlias.title', {
defaultMessage: 'Links',
});

export const EMBEDDABLE_DISPLAY_NAME = i18n.translate(
'navigationEmbeddable.embeddableDisplayName',
{
defaultMessage: 'links',
}
);
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,19 @@ export { LATEST_VERSION, CONTENT_ID } from '../constants';
export type { NavigationEmbeddableContentType } from '../types';

export type {
NavigationEmbeddableCrudTypes,
NavigationEmbeddableAttributes,
NavigationEmbeddableItem,
NavigationLinkType,
NavigationLayoutType,
NavigationEmbeddableLink,
NavigationEmbeddableItem,
NavigationEmbeddableCrudTypes,
NavigationEmbeddableAttributes,
} from './latest';

export { DASHBOARD_LINK_TYPE, EXTERNAL_LINK_TYPE } from './latest';
export {
EXTERNAL_LINK_TYPE,
DASHBOARD_LINK_TYPE,
NAV_VERTICAL_LAYOUT,
NAV_HORIZONTAL_LAYOUT,
} from './latest';

export * as NavigationEmbeddableV1 from './v1';
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,13 @@ import { schema } from '@kbn/config-schema';
import type { ContentManagementServicesDefinition as ServicesDefinition } from '@kbn/object-versioning';
import {
savedObjectSchema,
objectTypeToGetResultSchema,
createOptionsSchemas,
updateOptionsSchema,
createResultSchema,
updateOptionsSchema,
createOptionsSchemas,
objectTypeToGetResultSchema,
} from '@kbn/content-management-utils';
import { DASHBOARD_LINK_TYPE, EXTERNAL_LINK_TYPE } from '.';
import { NAV_HORIZONTAL_LAYOUT, NAV_VERTICAL_LAYOUT } from './constants';

const navigationEmbeddableLinkSchema = schema.object({
id: schema.string(),
Expand All @@ -30,6 +31,9 @@ const navigationEmbeddableAttributesSchema = schema.object(
title: schema.string(),
description: schema.maybe(schema.string()),
links: schema.maybe(schema.arrayOf(navigationEmbeddableLinkSchema)),
layout: schema.maybe(
schema.oneOf([schema.literal(NAV_HORIZONTAL_LAYOUT), schema.literal(NAV_VERTICAL_LAYOUT)])
),
},
{ unknowns: 'forbid' }
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@
*/

/**
* Dashboard to dashboard links
* Link types
*/
export const DASHBOARD_LINK_TYPE = 'dashboardLink';
export const EXTERNAL_LINK_TYPE = 'externalLink';

/**
* External URL links
* Layout options
*/
export const EXTERNAL_LINK_TYPE = 'externalLink';
export const NAV_HORIZONTAL_LAYOUT = 'horizontal';
export const NAV_VERTICAL_LAYOUT = 'vertical';
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,13 @@ export type {
NavigationEmbeddableCrudTypes,
NavigationEmbeddableAttributes,
NavigationEmbeddableLink,
NavigationLayoutType,
NavigationLinkType,
} from './types';
export type NavigationEmbeddableItem = NavigationEmbeddableCrudTypes['Item'];
export { DASHBOARD_LINK_TYPE, EXTERNAL_LINK_TYPE } from './constants';
export {
EXTERNAL_LINK_TYPE,
DASHBOARD_LINK_TYPE,
NAV_VERTICAL_LAYOUT,
NAV_HORIZONTAL_LAYOUT,
} from './constants';
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,12 @@ import type {
SavedObjectUpdateOptions,
} from '@kbn/content-management-utils';
import { NavigationEmbeddableContentType } from '../../types';
import { DASHBOARD_LINK_TYPE, EXTERNAL_LINK_TYPE } from './constants';
import {
DASHBOARD_LINK_TYPE,
EXTERNAL_LINK_TYPE,
NAV_HORIZONTAL_LAYOUT,
NAV_VERTICAL_LAYOUT,
} from './constants';

export type NavigationEmbeddableCrudTypes = ContentManagementCrudTypes<
NavigationEmbeddableContentType,
Expand All @@ -38,9 +43,12 @@ export interface NavigationEmbeddableLink {
order: number;
}

export type NavigationLayoutType = typeof NAV_HORIZONTAL_LAYOUT | typeof NAV_VERTICAL_LAYOUT;

// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
export type NavigationEmbeddableAttributes = {
title: string;
description?: string;
links?: NavigationEmbeddableLink[];
layout?: NavigationLayoutType;
};
Loading

0 comments on commit f0ebcb2

Please sign in to comment.