Skip to content

Commit

Permalink
[Dashboard Usability] Conditionally auto focus on title input in pane…
Browse files Browse the repository at this point in the history 
…l settings flyout (elastic#173777)

Closes elastic#170786

## Summary

Currently, in order to change a panel title, the most efficient way to
do this (assuming the dashboard is already in edit mode) is to click on
the panel title -> click on the title input in the flyout -> click save
at the bottom of the flyout. Notice that this process currently takes
**three** clicks, which can start to add up if you need to change
multiple titles in one session - so, in order to remove one click from
this process, I've made it so that the title input is **auto focused**
on when opening the settings flyout through the panel title (and not
when it is opened from the context menu).

> [!NOTE]
> I chose to make this auto-focus behaviour conditional on how the
flyout was opened because, from an a11y perspective, it can be jarring
to have your focus taken out of the natural element order (as noted in
the [EUI
docs](https://eui.elastic.co/#/layout/popover#setting-an-initial-focus)).
It feels natural that, in order to change the panel title, you click on
it - so, auto focusing on the title input makes sense, even when using
keyboard controls. However, if you are opening the settings flyout via
the context menu, it is **less likely** that the goal is to change the
title - so, forcing the focus could feel unnatural in this case.


I added tests for this new auto focus behaviour and, since I was
interested in learning a bit more about RTL, I also added a few other
tests for the dashboard panel components. As part of this, I migrated a
few functional tests to unit tests, since this is a faster and more
reliable way to test certain rendering conditionals.

### Video


https://github.com/elastic/kibana/assets/8698078/229c1303-c81d-46b8-a567-76885361d9fa


### 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
- [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 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 <42973632+kibanamachine@users.noreply.github.com>
  • Loading branch information
@Heenawter @kibanamachine
Heenawter and kibanamachine committed Jan 4, 2024
1 parent 46a5854 commit 1330168
Show file tree
Hide file tree
Showing 19 changed files with 601 additions and 263 deletions.
29 changes: 10 additions & 19 deletions src/plugins/embeddable/public/embeddable_panel/embeddable_panel.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,23 @@
* Side Public License, v 1.
*/

import { isNil } from 'lodash';
import { EuiFlexGroup, EuiFlexItem, EuiPanel, htmlIdGenerator } from '@elastic/eui';
import classNames from 'classnames';
import { distinct, map } from 'rxjs';
import { isNil } from 'lodash';
import React, { ReactNode, useEffect, useMemo, useState } from 'react';
import { EuiFlexGroup, EuiFlexItem, EuiPanel, htmlIdGenerator } from '@elastic/eui';
import { distinct, map } from 'rxjs';

import { UI_SETTINGS } from '@kbn/data-plugin/common';
import { PanelLoader } from '@kbn/panel-loader';
import { core, embeddableStart, inspector } from '../kibana_services';
import { EmbeddableErrorHandler, EmbeddableOutput, ViewMode } from '../lib';
import { EmbeddablePanelError } from './embeddable_panel_error';
import {
CustomizePanelAction,
EditPanelAction,
RemovePanelAction,
InspectPanelAction,
CustomizePanelAction,
RemovePanelAction,
} from './panel_actions';
import { EmbeddablePanelHeader } from './panel_header/embeddable_panel_header';
import {
EmbeddablePhase,
EmbeddablePhaseEvent,
Expand All @@ -30,10 +33,6 @@ import {
useSelectFromEmbeddableInput,
useSelectFromEmbeddableOutput,
} from './use_select_from_embeddable';
import { EmbeddablePanelError } from './embeddable_panel_error';
import { core, embeddableStart, inspector } from '../kibana_services';
import { ViewMode, EmbeddableErrorHandler, EmbeddableOutput } from '../lib';
import { EmbeddablePanelHeader } from './panel_header/embeddable_panel_header';

const getEventStatus = (output: EmbeddableOutput): EmbeddablePhase => {
if (!isNil(output.error)) {
Expand Down Expand Up @@ -61,8 +60,6 @@ export const EmbeddablePanel = (panelProps: UnwrappedEmbeddablePanelProps) => {
* bypass the trigger registry.
*/
const universalActions = useMemo<PanelUniversalActions>(() => {
const commonlyUsedRanges = core.uiSettings.get(UI_SETTINGS.TIMEPICKER_QUICK_RANGES);
const dateFormat = core.uiSettings.get(UI_SETTINGS.DATE_FORMAT);
const stateTransfer = embeddableStart.getStateTransfer();
const editPanel = new EditPanelAction(
embeddableStart.getEmbeddableFactory,
Expand All @@ -71,13 +68,7 @@ export const EmbeddablePanel = (panelProps: UnwrappedEmbeddablePanelProps) => {
);

const actions: PanelUniversalActions = {
customizePanel: new CustomizePanelAction(
core.overlays,
core.theme,
editPanel,
commonlyUsedRanges,
dateFormat
),
customizePanel: new CustomizePanelAction(editPanel),
removePanel: new RemovePanelAction(),
editPanel,
};
Expand Down
2 changes: 1 addition & 1 deletion ...le/public/embeddable_panel/panel_actions/customize_panel_action/can_inherit_time_range.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@

import type { TimeRange } from '@kbn/es-query';

import { TimeRangeInput } from './customize_panel_action';
import { Embeddable, IContainer, ContainerInput } from '../../..';
import { TimeRangeInput } from './time_range_helpers';

interface ContainerTimeRangeInput extends ContainerInput<TimeRangeInput> {
timeRange: TimeRange;
Expand Down
31 changes: 5 additions & 26 deletions ...lic/embeddable_panel/panel_actions/customize_panel_action/custom_time_range_badge.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,13 @@
* Side Public License, v 1.
*/

import { themeServiceMock } from '@kbn/core-theme-browser-mocks';
import { overlayServiceMock } from '@kbn/core-overlays-browser-mocks';

import {
TimeRangeEmbeddable,
TimeRangeContainer,
TimeRangeEmbeddable,
TIME_RANGE_EMBEDDABLE,
} from '../../../lib/test_samples/embeddables';
import { CustomTimeRangeBadge } from './custom_time_range_badge';
import { EditPanelAction } from '../edit_panel_action/edit_panel_action';
import { CustomTimeRangeBadge } from './custom_time_range_badge';

const editPanelAction = {
execute: jest.fn(),
Expand All @@ -42,13 +39,7 @@ test(`badge is not compatible with embeddable that inherits from parent`, async

const child = container.getChild<TimeRangeEmbeddable>('1');

const compatible = await new CustomTimeRangeBadge(
overlayServiceMock.createStartContract(),
themeServiceMock.createStartContract(),
editPanelAction,
[],
'MM YYYY'
).isCompatible({
const compatible = await new CustomTimeRangeBadge(editPanelAction, 'MM YYYY').isCompatible({
embeddable: child,
});
expect(compatible).toBe(false);
Expand Down Expand Up @@ -76,13 +67,7 @@ test(`badge is compatible with embeddable that has custom time range`, async ()

const child = container.getChild<TimeRangeEmbeddable>('1');

const compatible = await new CustomTimeRangeBadge(
overlayServiceMock.createStartContract(),
themeServiceMock.createStartContract(),
editPanelAction,
[],
'MM YYYY'
).isCompatible({
const compatible = await new CustomTimeRangeBadge(editPanelAction, 'MM YYYY').isCompatible({
embeddable: child,
});
expect(compatible).toBe(true);
Expand All @@ -109,13 +94,7 @@ test('Attempting to execute on incompatible embeddable throws an error', async (

const child = container.getChild<TimeRangeEmbeddable>('1');

const badge = await new CustomTimeRangeBadge(
overlayServiceMock.createStartContract(),
themeServiceMock.createStartContract(),
editPanelAction,
[],
'MM YYYY'
);
const badge = await new CustomTimeRangeBadge(editPanelAction, 'MM YYYY');

async function check() {
await badge.execute({ embeddable: child });
Expand Down
12 changes: 10 additions & 2 deletions .../public/embeddable_panel/panel_actions/customize_panel_action/custom_time_range_badge.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ import { PrettyDuration } from '@elastic/eui';
import { renderToString } from 'react-dom/server';
import { Action } from '@kbn/ui-actions-plugin/public';

import { Embeddable } from '../../..';
import { EditPanelAction, Embeddable } from '../../..';
import { doesInheritTimeRange } from './does_inherit_time_range';
import { TimeRangeInput, hasTimeRange, CustomizePanelAction } from './customize_panel_action';
import { CustomizePanelAction } from './customize_panel_action';
import { hasTimeRange, TimeRangeInput } from './time_range_helpers';

export const CUSTOM_TIME_RANGE_BADGE = 'CUSTOM_TIME_RANGE_BADGE';

Expand All @@ -29,6 +30,13 @@ export class CustomTimeRangeBadge
public readonly id = CUSTOM_TIME_RANGE_BADGE;
public order = 7;

constructor(
protected readonly editPanel: EditPanelAction,
protected readonly dateFormat?: string
) {
super(editPanel);
}

public getDisplayName({ embeddable }: TimeBadgeActionContext) {
return renderToString(
<PrettyDuration
Expand Down
15 changes: 6 additions & 9 deletions ...blic/embeddable_panel/panel_actions/customize_panel_action/customize_panel_action.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,27 +6,24 @@
* Side Public License, v 1.
*/

import { overlayServiceMock } from '@kbn/core-overlays-browser-mocks';
import { themeServiceMock } from '@kbn/core-theme-browser-mocks';
import { Container, isErrorEmbeddable } from '../../..';
import { CustomizePanelAction } from './customize_panel_action';
import {
ContactCardEmbeddable,
ContactCardEmbeddableInput,
ContactCardEmbeddableOutput,
} from '../../../lib/test_samples/embeddables/contact_card/contact_card_embeddable';
import {
CONTACT_CARD_EMBEDDABLE,
ContactCardEmbeddableFactory,
CONTACT_CARD_EMBEDDABLE,
} from '../../../lib/test_samples/embeddables/contact_card/contact_card_embeddable_factory';
import { HelloWorldContainer } from '../../../lib/test_samples/embeddables/hello_world_container';
import { embeddablePluginMock } from '../../../mocks';
import { EditPanelAction } from '../edit_panel_action/edit_panel_action';
import { CustomizePanelAction } from './customize_panel_action';
import * as openCustomizePanel from './open_customize_panel';

let container: Container;
let embeddable: ContactCardEmbeddable;
const overlays = overlayServiceMock.createStartContract();
const theme = themeServiceMock.createStartContract();
const editPanelActionMock = { execute: jest.fn() } as unknown as EditPanelAction;

function createHelloWorldContainer(input = { id: '123', panels: {} }) {
Expand Down Expand Up @@ -59,9 +56,9 @@ beforeAll(async () => {
});

test('execute should open flyout', async () => {
const customizePanelAction = new CustomizePanelAction(overlays, theme, editPanelActionMock);
const spy = jest.spyOn(overlays, 'openFlyout');
await customizePanelAction.execute({ embeddable });
const customizePanelAction = new CustomizePanelAction(editPanelActionMock);

const spy = jest.spyOn(openCustomizePanel, 'openCustomizePanelFlyout');
await customizePanelAction.execute({ embeddable });
expect(spy).toHaveBeenCalled();
});
114 changes: 7 additions & 107 deletions ...e/public/embeddable_panel/panel_actions/customize_panel_action/customize_panel_action.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,48 +6,16 @@
* Side Public License, v 1.
*/

import React from 'react';
import { i18n } from '@kbn/i18n';
import { TimeRange } from '@kbn/es-query';
import { createKibanaReactContext } from '@kbn/kibana-react-plugin/public';
import { OverlayStart, ThemeServiceStart } from '@kbn/core/public';
import { toMountPoint } from '@kbn/react-kibana-mount';
import { Action, IncompatibleActionError } from '@kbn/ui-actions-plugin/public';

import { core } from '../../../kibana_services';
import {
IEmbeddable,
Embeddable,
EmbeddableInput,
EmbeddableOutput,
EditPanelAction,
} from '../../..';
import { ViewMode, CommonlyUsedRange } from '../../../lib/types';
import { tracksOverlays } from '../track_overlays';
import { CustomizePanelEditor } from './customize_panel_editor';
import { EditPanelAction, Embeddable, IEmbeddable } from '../../..';
import { ViewMode } from '../../../lib/types';
import { openCustomizePanelFlyout } from './open_customize_panel';
import { isTimeRangeCompatible, TimeRangeInput } from './time_range_helpers';

export const ACTION_CUSTOMIZE_PANEL = 'ACTION_CUSTOMIZE_PANEL';

const VISUALIZE_EMBEDDABLE_TYPE = 'visualization';

type VisualizeEmbeddable = IEmbeddable<{ id: string }, EmbeddableOutput & { visTypeName: string }>;

function isVisualizeEmbeddable(
embeddable: IEmbeddable | VisualizeEmbeddable
): embeddable is VisualizeEmbeddable {
return embeddable.type === VISUALIZE_EMBEDDABLE_TYPE;
}

export interface TimeRangeInput extends EmbeddableInput {
timeRange: TimeRange;
}

export function hasTimeRange(
embeddable: IEmbeddable | Embeddable<TimeRangeInput>
): embeddable is Embeddable<TimeRangeInput> {
return (embeddable as Embeddable<TimeRangeInput>).getInput().timeRange !== undefined;
}

export interface CustomizePanelActionContext {
embeddable: IEmbeddable | Embeddable<TimeRangeInput>;
}
Expand All @@ -57,35 +25,7 @@ export class CustomizePanelAction implements Action<CustomizePanelActionContext>
public id = ACTION_CUSTOMIZE_PANEL;
public order = 40;

constructor(
protected readonly overlays: OverlayStart,
protected readonly theme: ThemeServiceStart,
protected readonly editPanel: EditPanelAction,
protected readonly commonlyUsedRanges?: CommonlyUsedRange[],
protected readonly dateFormat?: string
) {}

protected isTimeRangeCompatible({ embeddable }: CustomizePanelActionContext): boolean {
const isInputControl =
isVisualizeEmbeddable(embeddable) &&
(embeddable as VisualizeEmbeddable).getOutput().visTypeName === 'input_control_vis';

const isMarkdown =
isVisualizeEmbeddable(embeddable) &&
(embeddable as VisualizeEmbeddable).getOutput().visTypeName === 'markdown';

const isImage = embeddable.type === 'image';
const isNavigation = embeddable.type === 'navigation';

return Boolean(
embeddable &&
hasTimeRange(embeddable) &&
!isInputControl &&
!isMarkdown &&
!isImage &&
!isNavigation
);
}
constructor(protected readonly editPanel: EditPanelAction) {}

public getDisplayName({ embeddable }: CustomizePanelActionContext): string {
return i18n.translate('embeddableApi.customizePanel.action.displayName', {
Expand All @@ -100,7 +40,7 @@ export class CustomizePanelAction implements Action<CustomizePanelActionContext>
public async isCompatible({ embeddable }: CustomizePanelActionContext) {
// It should be possible to customize just the time range in View mode
return (
embeddable.getInput().viewMode === ViewMode.EDIT || this.isTimeRangeCompatible({ embeddable })
embeddable.getInput().viewMode === ViewMode.EDIT || isTimeRangeCompatible({ embeddable })
);
}

Expand All @@ -109,46 +49,6 @@ export class CustomizePanelAction implements Action<CustomizePanelActionContext>
if (!isCompatible) {
throw new IncompatibleActionError();
}

// send the overlay ref to the root embeddable if it is capable of tracking overlays
const rootEmbeddable = embeddable.getRoot();
const overlayTracker = tracksOverlays(rootEmbeddable) ? rootEmbeddable : undefined;

const { Provider: KibanaReactContextProvider } = createKibanaReactContext({
uiSettings: core.uiSettings,
});

const onEdit = () => {
this.editPanel.execute({ embeddable });
};

const handle = this.overlays.openFlyout(
toMountPoint(
<KibanaReactContextProvider>
<CustomizePanelEditor
embeddable={embeddable}
timeRangeCompatible={this.isTimeRangeCompatible({ embeddable })}
dateFormat={this.dateFormat}
commonlyUsedRanges={this.commonlyUsedRanges}
onClose={() => {
if (overlayTracker) overlayTracker.clearOverlays();
handle.close();
}}
onEdit={onEdit}
/>
</KibanaReactContextProvider>,
{ theme: this.theme, i18n: core.i18n }
),
{
size: 's',
'data-test-subj': 'customizePanel',
onClose: (overlayRef) => {
if (overlayTracker) overlayTracker.clearOverlays();
overlayRef.close();
},
maxWidth: true,
}
);
overlayTracker?.openOverlay(handle);
openCustomizePanelFlyout({ editPanel: this.editPanel, embeddable });
}
}

0 comments on commit 1330168

Please sign in to comment.