From 7c1c7970ae7f321ab33b793ed20dcaf6b834d9e4 Mon Sep 17 00:00:00 2001 From: jeramysoucy Date: Wed, 15 Jun 2022 14:34:40 +0200 Subject: [PATCH 01/29] Refactor of spaces pop-over using EuiSelectable. --- .../nav_control/components/spaces_menu.tsx | 251 +++++++----------- .../spaces/public/nav_control/nav_control.tsx | 1 + .../nav_control/nav_control_popover.tsx | 2 + 3 files changed, 106 insertions(+), 148 deletions(-) diff --git a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx index 6f5158423ca517..f99246290386e0 100644 --- a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx +++ b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx @@ -7,30 +7,22 @@ import './spaces_menu.scss'; -import { - EuiContextMenuItem, - EuiContextMenuPanel, - EuiFieldSearch, - EuiLoadingContent, - EuiLoadingSpinner, - EuiText, -} from '@elastic/eui'; -import type { ReactElement } from 'react'; -import React, { Component, lazy, Suspense } from 'react'; +import { EuiAvatar, EuiPopoverFooter, EuiPopoverTitle, EuiSelectable } from '@elastic/eui'; +import type { EuiSelectableOptionCheckedType } from '@elastic/eui/src/components/selectable/selectable_option'; +import React, { Component } from 'react'; import type { ApplicationStart, Capabilities } from '@kbn/core/public'; import type { InjectedIntl } from '@kbn/i18n-react'; -import { FormattedMessage, injectI18n } from '@kbn/i18n-react'; +import { injectI18n } from '@kbn/i18n-react'; import type { Space } from '../../../common'; import { addSpaceIdToPath, ENTER_SPACE_PATH, SPACE_SEARCH_COUNT_THRESHOLD } from '../../../common'; -import { getSpaceAvatarComponent } from '../../space_avatar'; import { ManageSpacesButton } from './manage_spaces_button'; // No need to wrap LazySpaceAvatar in an error boundary, because it is one of the first chunks loaded when opening Kibana. -const LazySpaceAvatar = lazy(() => - getSpaceAvatarComponent().then((component) => ({ default: component })) -); +// const LazySpaceAvatar = lazy(() => +// getSpaceAvatarComponent().then((component) => ({ default: component })) +// ); interface Props { id: string; @@ -41,26 +33,91 @@ interface Props { intl: InjectedIntl; capabilities: Capabilities; navigateToApp: ApplicationStart['navigateToApp']; + navigateToUrl: ApplicationStart['navigateToUrl']; } interface State { + // selectedSpace: Space; searchTerm: string; allowSpacesListFocus: boolean; } +/** + * Selectable Space Item for display and navigation purposes. + */ +interface SelectableSpaceItem { + label: string; + key: string; + prepend: JSX.Element; + checked: EuiSelectableOptionCheckedType; + 'data-test-subj': string; + className: string; +} + class SpacesMenuUI extends Component { + // ToDo: removed unused state members public state = { + // selectedSpace: this.props.spaces[0], searchTerm: '', allowSpacesListFocus: false, }; public render() { - const { intl, isLoading } = this.props; - const { searchTerm } = this.state; + const { intl, isLoading, spaces, serverBasePath, navigateToUrl } = this.props; + // const { searchTerm } = this.state; + + // ToDo: how to clean this up into functions/members? + const spaceItems: SelectableSpaceItem[] = isLoading + ? [ + { + label: '', + key: '', + prepend: <>, + checked: 'off', + 'data-test-subj': '', + className: 'selectable-space-item', + }, + ] + : // this.getVisibleSpaces(searchTerm).map((space) => { + spaces.map((space) => { + return { + label: space.name, + key: space.id, + prepend: ( + + ), + checked: 'off', // ToDo: set 'on' if this space ID matches the one we're in? + 'data-test-subj': `${space.id}-selectableSpaceItem`, + className: 'selectableSpaceItem', + }; + }); + + const onChange = (newItems: SelectableSpaceItem[]) => { + newItems.forEach((element: { label: any; checked: any }) => { + console.log(`**** ${element.label}, Checked: ${element.checked}`); + }); + const selectedSpaceItem = newItems.filter((item) => item.checked?.includes('on'))[0]; - const items = isLoading - ? [1, 2, 3].map(this.renderPlaceholderMenuItem) - : this.getVisibleSpaces(searchTerm).map(this.renderSpaceMenuItem); + if (!!selectedSpaceItem) { + // selectedSpaceItem.checked = 'off'; + // const selectedSpace = spaces.filter((space) => selectedSpaceItem.label === space.name)[0]; + + // ToDo: handle options (middle click or cmd/ctrl (new tab), shift click (new window)) + const urlToSelectedSpace = addSpaceIdToPath( + serverBasePath, + selectedSpaceItem.key, // selectedSpace.id + ENTER_SPACE_PATH + ); + // // Force full page reload (usually not a good idea, but we need to in order to change spaces) + navigateToUrl(urlToSelectedSpace); + } + }; const panelProps = { id: this.props.id, @@ -71,102 +128,34 @@ class SpacesMenuUI extends Component { }), }; - if (this.props.spaces.length >= SPACE_SEARCH_COUNT_THRESHOLD) { - return ( - - {this.renderSearchField()} - {this.renderSpacesListPanel(items, searchTerm)} - {this.renderManageButton()} - - ); - } - - items.push(this.renderManageButton()); - - return ; - } - - private getVisibleSpaces = (searchTerm: string): Space[] => { - const { spaces } = this.props; - - let filteredSpaces = spaces; - if (searchTerm) { - filteredSpaces = spaces.filter((space) => { - const { name, description = '' } = space; - return ( - name.toLowerCase().indexOf(searchTerm) >= 0 || - description.toLowerCase().indexOf(searchTerm) >= 0 - ); - }); - } - - return filteredSpaces; - }; - - private renderSpacesListPanel = (items: ReactElement[], searchTerm: string) => { - if (items.length === 0) { - return ( - - - - ); - } - return ( - - ); - }; - - private renderSearchField = () => { - const { intl } = this.props; - return ( -
- { - - } -
+ = SPACE_SEARCH_COUNT_THRESHOLD} + // ToDo: I had to comment this out, due to it causing an error with EuiSelectable, but I wasn't quite sure I understood... + // searchProps={{ + // placeholder: 'Find a space', + // compressed: true, + // }} + options={spaceItems} + singleSelection={true} + style={{ width: 300 }} + onChange={onChange} + listProps={{ + rowHeight: 40, + showIcons: false, + }} + > + {(list, search) => ( + <> + {search || 'Your spaces'} + {list} + {this.renderManageButton()} + + )} + ); - }; - - private onSearchKeyDown = (e: any) => { - // 9: tab - // 13: enter - // 40: arrow-down - const focusableKeyCodes = [9, 13, 40]; - - const keyCode = e.keyCode; - if (focusableKeyCodes.includes(keyCode)) { - // Allows the spaces list panel to receive focus. This enables keyboard and screen reader navigation - this.setState({ - allowSpacesListFocus: true, - }); - } - }; - - private onSearchFocus = () => { - this.setState({ - allowSpacesListFocus: false, - }); - }; + } private renderManageButton = () => { return ( @@ -180,40 +169,6 @@ class SpacesMenuUI extends Component { /> ); }; - - private onSearch = (searchTerm: string) => { - this.setState({ - searchTerm: searchTerm.trim().toLowerCase(), - }); - }; - - private renderSpaceMenuItem = (space: Space): JSX.Element => { - const icon = ( - }> - - - ); - return ( - - {space.name} - - ); - }; - - private renderPlaceholderMenuItem = (key: string | number): JSX.Element => { - return ( - - - - ); - }; } export const SpacesMenu = injectI18n(SpacesMenuUI); diff --git a/x-pack/plugins/spaces/public/nav_control/nav_control.tsx b/x-pack/plugins/spaces/public/nav_control/nav_control.tsx index cf12634ae188e1..80b08bdd5f8f89 100644 --- a/x-pack/plugins/spaces/public/nav_control/nav_control.tsx +++ b/x-pack/plugins/spaces/public/nav_control/nav_control.tsx @@ -40,6 +40,7 @@ export function initSpacesNavControl(spacesManager: SpacesManager, core: CoreSta anchorPosition="downLeft" capabilities={core.application.capabilities} navigateToApp={core.application.navigateToApp} + navigateToUrl={core.application.navigateToUrl} /> diff --git a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx index 953c40e9463649..2c8171d686de06 100644 --- a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx +++ b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx @@ -28,6 +28,7 @@ interface Props { anchorPosition: PopoverAnchorPosition; capabilities: Capabilities; navigateToApp: ApplicationStart['navigateToApp']; + navigateToUrl: ApplicationStart['navigateToUrl']; serverBasePath: string; } @@ -92,6 +93,7 @@ export class NavControlPopover extends Component { onManageSpacesClick={this.toggleSpaceSelector} capabilities={this.props.capabilities} navigateToApp={this.props.navigateToApp} + navigateToUrl={this.props.navigateToUrl} /> ); } From 7a9df0d825c4bfafb335c033868dd7f1a1b804a6 Mon Sep 17 00:00:00 2001 From: jeramysoucy Date: Wed, 15 Jun 2022 17:45:02 +0200 Subject: [PATCH 02/29] Updated to use Eui type instead of interface. --- .../nav_control/components/spaces_menu.tsx | 91 ++++++++----------- 1 file changed, 38 insertions(+), 53 deletions(-) diff --git a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx index f99246290386e0..316f9025e3fca2 100644 --- a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx +++ b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx @@ -8,8 +8,11 @@ import './spaces_menu.scss'; import { EuiAvatar, EuiPopoverFooter, EuiPopoverTitle, EuiSelectable } from '@elastic/eui'; -import type { EuiSelectableOptionCheckedType } from '@elastic/eui/src/components/selectable/selectable_option'; -import React, { Component } from 'react'; +import type { + EuiSelectableOption, + // EuiSelectableOptionCheckedType, +} from '@elastic/eui/src/components/selectable/selectable_option'; +import React, { Component, lazy } from 'react'; import type { ApplicationStart, Capabilities } from '@kbn/core/public'; import type { InjectedIntl } from '@kbn/i18n-react'; @@ -17,6 +20,7 @@ import { injectI18n } from '@kbn/i18n-react'; import type { Space } from '../../../common'; import { addSpaceIdToPath, ENTER_SPACE_PATH, SPACE_SEARCH_COUNT_THRESHOLD } from '../../../common'; +// import { getSpaceAvatarComponent } from '../../space_avatar'; import { ManageSpacesButton } from './manage_spaces_button'; // No need to wrap LazySpaceAvatar in an error boundary, because it is one of the first chunks loaded when opening Kibana. @@ -42,18 +46,6 @@ interface State { allowSpacesListFocus: boolean; } -/** - * Selectable Space Item for display and navigation purposes. - */ -interface SelectableSpaceItem { - label: string; - key: string; - prepend: JSX.Element; - checked: EuiSelectableOptionCheckedType; - 'data-test-subj': string; - className: string; -} - class SpacesMenuUI extends Component { // ToDo: removed unused state members public state = { @@ -63,43 +55,31 @@ class SpacesMenuUI extends Component { }; public render() { - const { intl, isLoading, spaces, serverBasePath, navigateToUrl } = this.props; + const { intl, spaces, serverBasePath, navigateToUrl } = this.props; // const { searchTerm } = this.state; // ToDo: how to clean this up into functions/members? - const spaceItems: SelectableSpaceItem[] = isLoading - ? [ - { - label: '', - key: '', - prepend: <>, - checked: 'off', - 'data-test-subj': '', - className: 'selectable-space-item', - }, - ] - : // this.getVisibleSpaces(searchTerm).map((space) => { - spaces.map((space) => { - return { - label: space.name, - key: space.id, - prepend: ( - - ), - checked: 'off', // ToDo: set 'on' if this space ID matches the one we're in? - 'data-test-subj': `${space.id}-selectableSpaceItem`, - className: 'selectableSpaceItem', - }; - }); - - const onChange = (newItems: SelectableSpaceItem[]) => { - newItems.forEach((element: { label: any; checked: any }) => { + const spaceItems: EuiSelectableOption[] = spaces.map((space) => { + return { + label: space.name, + key: space.id, + prepend: ( + + ), + checked: 'off', // ToDo: set 'on' if this space ID matches the one we're in? + 'data-test-subj': `${space.id}-selectableSpaceItem`, + className: 'selectableSpaceItem', + }; + }); + + const onChange = (newItems: EuiSelectableOption[]) => { + newItems.forEach((element) => { console.log(`**** ${element.label}, Checked: ${element.checked}`); }); const selectedSpaceItem = newItems.filter((item) => item.checked?.includes('on'))[0]; @@ -131,12 +111,17 @@ class SpacesMenuUI extends Component { return ( = SPACE_SEARCH_COUNT_THRESHOLD} - // ToDo: I had to comment this out, due to it causing an error with EuiSelectable, but I wasn't quite sure I understood... - // searchProps={{ - // placeholder: 'Find a space', - // compressed: true, - // }} + // ToDo: find a way to do this more robustly, i.e. not use 'any' + searchProps={ + this.props.spaces.length >= SPACE_SEARCH_COUNT_THRESHOLD + ? ({ + placeholder: 'Find a space', + compressed: true, + } as any) + : undefined + } options={spaceItems} singleSelection={true} style={{ width: 300 }} From d7b2ac28b9afb324def663b4a69e87ef5cb23f1d Mon Sep 17 00:00:00 2001 From: jeramysoucy Date: Thu, 16 Jun 2022 08:51:18 +0200 Subject: [PATCH 03/29] Reorganization of code, began to add EuiSelectableOnChangeEvent usage. --- .../nav_control/components/spaces_menu.tsx | 120 +++++++++++------- 1 file changed, 71 insertions(+), 49 deletions(-) diff --git a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx index 316f9025e3fca2..bfdbf81d214e7e 100644 --- a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx +++ b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx @@ -8,11 +8,8 @@ import './spaces_menu.scss'; import { EuiAvatar, EuiPopoverFooter, EuiPopoverTitle, EuiSelectable } from '@elastic/eui'; -import type { - EuiSelectableOption, - // EuiSelectableOptionCheckedType, -} from '@elastic/eui/src/components/selectable/selectable_option'; -import React, { Component, lazy } from 'react'; +import type { EuiSelectableOption } from '@elastic/eui/src/components/selectable/selectable_option'; +import React, { Component } from 'react'; import type { ApplicationStart, Capabilities } from '@kbn/core/public'; import type { InjectedIntl } from '@kbn/i18n-react'; @@ -55,54 +52,15 @@ class SpacesMenuUI extends Component { }; public render() { - const { intl, spaces, serverBasePath, navigateToUrl } = this.props; + // const { intl, spaces, serverBasePath, navigateToUrl } = this.props; // const { searchTerm } = this.state; - // ToDo: how to clean this up into functions/members? - const spaceItems: EuiSelectableOption[] = spaces.map((space) => { - return { - label: space.name, - key: space.id, - prepend: ( - - ), - checked: 'off', // ToDo: set 'on' if this space ID matches the one we're in? - 'data-test-subj': `${space.id}-selectableSpaceItem`, - className: 'selectableSpaceItem', - }; - }); - - const onChange = (newItems: EuiSelectableOption[]) => { - newItems.forEach((element) => { - console.log(`**** ${element.label}, Checked: ${element.checked}`); - }); - const selectedSpaceItem = newItems.filter((item) => item.checked?.includes('on'))[0]; - - if (!!selectedSpaceItem) { - // selectedSpaceItem.checked = 'off'; - // const selectedSpace = spaces.filter((space) => selectedSpaceItem.label === space.name)[0]; - - // ToDo: handle options (middle click or cmd/ctrl (new tab), shift click (new window)) - const urlToSelectedSpace = addSpaceIdToPath( - serverBasePath, - selectedSpaceItem.key, // selectedSpace.id - ENTER_SPACE_PATH - ); - // // Force full page reload (usually not a good idea, but we need to in order to change spaces) - navigateToUrl(urlToSelectedSpace); - } - }; + const spaceMenuOptions: EuiSelectableOption[] = this.getSpaceOptions(); const panelProps = { id: this.props.id, className: 'spcMenu', - title: intl.formatMessage({ + title: this.props.intl.formatMessage({ id: 'xpack.spaces.navControl.spacesMenu.changeCurrentSpaceTitle', defaultMessage: 'Change current space', }), @@ -122,10 +80,10 @@ class SpacesMenuUI extends Component { } as any) : undefined } - options={spaceItems} + options={spaceMenuOptions} singleSelection={true} style={{ width: 300 }} - onChange={onChange} + onChange={this.spaceSelectionChange} listProps={{ rowHeight: 40, showIcons: false, @@ -142,6 +100,70 @@ class SpacesMenuUI extends Component { ); } + private getSpaceOptions = (): EuiSelectableOption[] => { + return this.props.spaces.map((space) => { + return { + label: space.name, + key: space.id, + prepend: ( + + ), + checked: undefined, // ToDo: set 'on' if this space ID matches the one we're in? + 'data-test-subj': `${space.id}-selectableSpaceItem`, + className: 'selectableSpaceItem', + }; + }); + }; + + private spaceSelectionChange = ( + newOptions: EuiSelectableOption[] + // event: KeyboardEvent | MouseEvent this (EuiSelectableOnChangeEvent) is not available yet (added to EUI 22 days ago)...may need to rebase + ) => { + // newOptions.forEach((option) => { + // console.log(`**** ${option.label}, Checked: ${option.checked}`); + // }); + const selectedSpaceItem = newOptions.filter((item) => item.checked === 'on')[0]; + + if (!!selectedSpaceItem) { + // selectedSpaceItem.checked = undefined; + // const selectedSpace = spaces.filter((space) => selectedSpaceItem.label === space.name)[0]; + + // ToDo: handle options (middle click or cmd/ctrl (new tab), shift click (new window)) + const urlToSelectedSpace = addSpaceIdToPath( + this.props.serverBasePath, + selectedSpaceItem.key, // selectedSpace.id + ENTER_SPACE_PATH + ); + + // console.log(`**** Event: ${event.type}`); + + // const mouseE = event as EuiSelectableOnChangeEvent; + // if (mouseE && mouseE.button === 1) { + // console.log(`**** MIDDLE CLICK`); + // } else if (mouseE && mouseE.ctrlKey) { + // console.log(`**** CTRL/CMD CLICK`); + // } else if (mouseE && mouseE.shiftKey) { + // console.log(`**** SHIFT CLICK`); + // } + + // const keyE = event as KeyboardEvent; + // if (keyE && keyE.ctrlKey === 1) { + // console.log(`**** CTRL/CMD ENTER`); + // } else if (keyE && keyE.shiftKey) { + // console.log(`**** SHIFT ENTER`); + // } + // Force full page reload (usually not a good idea, but we need to in order to change spaces) + // console.log(`**** URL: ${urlToSelectedSpace}`); + this.props.navigateToUrl(urlToSelectedSpace); + } + }; + private renderManageButton = () => { return ( Date: Thu, 16 Jun 2022 10:35:01 +0200 Subject: [PATCH 04/29] Repathed import after rebase --- .../spaces/public/nav_control/components/spaces_menu.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx index bfdbf81d214e7e..adcabe2ea95b87 100644 --- a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx +++ b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx @@ -8,7 +8,7 @@ import './spaces_menu.scss'; import { EuiAvatar, EuiPopoverFooter, EuiPopoverTitle, EuiSelectable } from '@elastic/eui'; -import type { EuiSelectableOption } from '@elastic/eui/src/components/selectable/selectable_option'; +import type { EuiSelectableOption } from '@elastic/eui/src/components/selectable'; import React, { Component } from 'react'; import type { ApplicationStart, Capabilities } from '@kbn/core/public'; @@ -123,7 +123,7 @@ class SpacesMenuUI extends Component { private spaceSelectionChange = ( newOptions: EuiSelectableOption[] - // event: KeyboardEvent | MouseEvent this (EuiSelectableOnChangeEvent) is not available yet (added to EUI 22 days ago)...may need to rebase + // event: KeyboardEvent | MouseEvent this (EuiSelectableOnChangeEvent) is not available yet (added to EUI 22 days ago)...waiting on 133927 ) => { // newOptions.forEach((option) => { // console.log(`**** ${option.label}, Checked: ${option.checked}`); From 733fbc1915a9e274520a4a2f6d435893fcadf4d1 Mon Sep 17 00:00:00 2001 From: jeramysoucy Date: Tue, 5 Jul 2022 12:55:09 +0200 Subject: [PATCH 05/29] Integrates the EuiSelectableOnChangeEvent argument to intercept keyboard and mouse event details (shift, ctrl, middle click). --- .../nav_control/components/spaces_menu.tsx | 55 ++++++++----------- 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx index adcabe2ea95b87..695d7215a80357 100644 --- a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx +++ b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx @@ -9,6 +9,7 @@ import './spaces_menu.scss'; import { EuiAvatar, EuiPopoverFooter, EuiPopoverTitle, EuiSelectable } from '@elastic/eui'; import type { EuiSelectableOption } from '@elastic/eui/src/components/selectable'; +import type { EuiSelectableOnChangeEvent } from '@elastic/eui/src/components/selectable/selectable'; import React, { Component } from 'react'; import type { ApplicationStart, Capabilities } from '@kbn/core/public'; @@ -38,7 +39,6 @@ interface Props { } interface State { - // selectedSpace: Space; searchTerm: string; allowSpacesListFocus: boolean; } @@ -46,7 +46,6 @@ interface State { class SpacesMenuUI extends Component { // ToDo: removed unused state members public state = { - // selectedSpace: this.props.spaces[0], searchTerm: '', allowSpacesListFocus: false, }; @@ -122,45 +121,39 @@ class SpacesMenuUI extends Component { }; private spaceSelectionChange = ( - newOptions: EuiSelectableOption[] - // event: KeyboardEvent | MouseEvent this (EuiSelectableOnChangeEvent) is not available yet (added to EUI 22 days ago)...waiting on 133927 + newOptions: EuiSelectableOption[], + event: EuiSelectableOnChangeEvent + // event: React.MouseEvent | React.KeyboardEvent ) => { - // newOptions.forEach((option) => { - // console.log(`**** ${option.label}, Checked: ${option.checked}`); - // }); const selectedSpaceItem = newOptions.filter((item) => item.checked === 'on')[0]; if (!!selectedSpaceItem) { - // selectedSpaceItem.checked = undefined; - // const selectedSpace = spaces.filter((space) => selectedSpaceItem.label === space.name)[0]; - - // ToDo: handle options (middle click or cmd/ctrl (new tab), shift click (new window)) const urlToSelectedSpace = addSpaceIdToPath( this.props.serverBasePath, selectedSpaceItem.key, // selectedSpace.id ENTER_SPACE_PATH ); - // console.log(`**** Event: ${event.type}`); - - // const mouseE = event as EuiSelectableOnChangeEvent; - // if (mouseE && mouseE.button === 1) { - // console.log(`**** MIDDLE CLICK`); - // } else if (mouseE && mouseE.ctrlKey) { - // console.log(`**** CTRL/CMD CLICK`); - // } else if (mouseE && mouseE.shiftKey) { - // console.log(`**** SHIFT CLICK`); - // } - - // const keyE = event as KeyboardEvent; - // if (keyE && keyE.ctrlKey === 1) { - // console.log(`**** CTRL/CMD ENTER`); - // } else if (keyE && keyE.shiftKey) { - // console.log(`**** SHIFT ENTER`); - // } - // Force full page reload (usually not a good idea, but we need to in order to change spaces) - // console.log(`**** URL: ${urlToSelectedSpace}`); - this.props.navigateToUrl(urlToSelectedSpace); + // ToDo: handle options (middle click or cmd/ctrl (new tab), shift click (new window)) + console.log(`**** Event Class: ${event.constructor.name}`); + + if (event.shiftKey) { + // Open in new window, shift is given priority over other modifiers + console.log(`**** SHIFT CLICK`); + window.open(urlToSelectedSpace); + } else if ( + (event instanceof KeyboardEvent && event.ctrlKey) || + (event instanceof MouseEvent && (event.button === 1 || event.ctrlKey)) + ) { + // Open in new tab - either a ctrl click or middle mouse button + console.log(`**** CTRL/CMD CLICK`); + // window.open(urlToSelectedSpace); // ToDo: replace with new tab + } else { + // Force full page reload (usually not a good idea, but we need to in order to change spaces) + // console.log(`**** URL: ${urlToSelectedSpace}`); + console.log(`**** NORMAL CLICK`); + // this.props.navigateToUrl(urlToSelectedSpace); + } } }; From de8a7d241708376a2f34f1b36b890b681debd935 Mon Sep 17 00:00:00 2001 From: jeramysoucy Date: Tue, 5 Jul 2022 15:28:11 +0200 Subject: [PATCH 06/29] Resolves missing property error in nav popover test. --- .../spaces/public/nav_control/nav_control_popover.test.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.test.tsx b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.test.tsx index 8e5aa0c6769fe0..0d6f46d06f904f 100644 --- a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.test.tsx +++ b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.test.tsx @@ -29,6 +29,7 @@ describe('NavControlPopover', () => { anchorPosition={'downRight'} capabilities={{ navLinks: {}, management: {}, catalogue: {}, spaces: { manage: true } }} navigateToApp={jest.fn()} + navigateToUrl={jest.fn()} /> ); expect(wrapper).toMatchSnapshot(); @@ -62,6 +63,7 @@ describe('NavControlPopover', () => { anchorPosition={'rightCenter'} capabilities={{ navLinks: {}, management: {}, catalogue: {}, spaces: { manage: true } }} navigateToApp={jest.fn()} + navigateToUrl={jest.fn()} /> ); From 6c176968e3bc4f7f7ab4bd3ea401d8b260dd1c20 Mon Sep 17 00:00:00 2001 From: jeramysoucy Date: Wed, 6 Jul 2022 08:24:06 +0200 Subject: [PATCH 07/29] Handles reintroduction of translated strings into empty and no match messages of the Spaces selectable. --- .../nav_control/components/spaces_menu.tsx | 55 +++++++++++++++---- 1 file changed, 44 insertions(+), 11 deletions(-) diff --git a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx index 695d7215a80357..a611457129127d 100644 --- a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx +++ b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx @@ -7,14 +7,14 @@ import './spaces_menu.scss'; -import { EuiAvatar, EuiPopoverFooter, EuiPopoverTitle, EuiSelectable } from '@elastic/eui'; +import { EuiAvatar, EuiPopoverFooter, EuiPopoverTitle, EuiSelectable, EuiText } from '@elastic/eui'; import type { EuiSelectableOption } from '@elastic/eui/src/components/selectable'; import type { EuiSelectableOnChangeEvent } from '@elastic/eui/src/components/selectable/selectable'; import React, { Component } from 'react'; import type { ApplicationStart, Capabilities } from '@kbn/core/public'; import type { InjectedIntl } from '@kbn/i18n-react'; -import { injectI18n } from '@kbn/i18n-react'; +import { FormattedMessage, injectI18n } from '@kbn/i18n-react'; import type { Space } from '../../../common'; import { addSpaceIdToPath, ENTER_SPACE_PATH, SPACE_SEARCH_COUNT_THRESHOLD } from '../../../common'; @@ -56,6 +56,15 @@ class SpacesMenuUI extends Component { const spaceMenuOptions: EuiSelectableOption[] = this.getSpaceOptions(); + const noMatchesMessage = ( + + + + ); + const panelProps = { id: this.props.id, className: 'spcMenu', @@ -79,14 +88,41 @@ class SpacesMenuUI extends Component { } as any) : undefined } + noMatchesMessage={noMatchesMessage} + emptyMessage={noMatchesMessage} options={spaceMenuOptions} singleSelection={true} style={{ width: 300 }} onChange={this.spaceSelectionChange} - listProps={{ - rowHeight: 40, - showIcons: false, - }} + listProps={{ showIcons: false }} + // onChange={(newOptions, event) => { + // const selectedSpaceItem = newOptions.filter((item) => item.checked === 'on')[0]; + + // if (!!selectedSpaceItem) { + // const urlToSelectedSpace = addSpaceIdToPath( + // this.props.serverBasePath, + // selectedSpaceItem.key, // selectedSpace.id + // ENTER_SPACE_PATH + // ); + + // console.log(event); + // console.log(`**** Event Class: ${event.constructor.name}`); + // // console.log(`**** Event Type: ${event.type}`); + + // if (event.shiftKey) { + // console.log(`**** SHIFT CLICK`); + // } else if (event.ctrlKey || event.metaKey) { + // // (event instanceof MouseEvent && (event.button === 1 || event.ctrlKey)) + // console.log(`**** CTRL/CMD CLICK`); + // } else { + // console.log(`**** NORMAL CLICK`); + // } + // } + // }} + // listProps={{ + // rowHeight: 40, + // showIcons: false, + // }} > {(list, search) => ( <> @@ -123,7 +159,6 @@ class SpacesMenuUI extends Component { private spaceSelectionChange = ( newOptions: EuiSelectableOption[], event: EuiSelectableOnChangeEvent - // event: React.MouseEvent | React.KeyboardEvent ) => { const selectedSpaceItem = newOptions.filter((item) => item.checked === 'on')[0]; @@ -136,15 +171,13 @@ class SpacesMenuUI extends Component { // ToDo: handle options (middle click or cmd/ctrl (new tab), shift click (new window)) console.log(`**** Event Class: ${event.constructor.name}`); + console.log(`**** Native Event: ${event.nativeEvent}`); if (event.shiftKey) { // Open in new window, shift is given priority over other modifiers console.log(`**** SHIFT CLICK`); window.open(urlToSelectedSpace); - } else if ( - (event instanceof KeyboardEvent && event.ctrlKey) || - (event instanceof MouseEvent && (event.button === 1 || event.ctrlKey)) - ) { + } else if (event.ctrlKey || event.metaKey) { // Open in new tab - either a ctrl click or middle mouse button console.log(`**** CTRL/CMD CLICK`); // window.open(urlToSelectedSpace); // ToDo: replace with new tab From 15d45f9eec1dff62202e612150ae221fbee69a41 Mon Sep 17 00:00:00 2001 From: jeramysoucy Date: Thu, 7 Jul 2022 14:58:40 +0200 Subject: [PATCH 08/29] Refactor of spaces_popover_list to use EuiSelectable, resolves keyboard navigation. --- .../spaces_popover_list.tsx | 187 +++++++----------- .../nav_control/components/spaces_menu.tsx | 99 ++++------ .../nav_control/nav_control_popover.tsx | 27 ++- 3 files changed, 132 insertions(+), 181 deletions(-) diff --git a/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx b/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx index 9ddc698ef2c2b9..252014aac7c91b 100644 --- a/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx +++ b/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx @@ -7,12 +7,13 @@ import './spaces_popover_list.scss'; +import type { EuiSelectableOption } from '@elastic/eui'; import { EuiButtonEmpty, - EuiContextMenuItem, - EuiContextMenuPanel, - EuiFieldSearch, + EuiFocusTrap, EuiPopover, + EuiPopoverTitle, + EuiSelectable, EuiText, } from '@elastic/eui'; import React, { Component, memo } from 'react'; @@ -56,9 +57,8 @@ export class SpacesPopoverList extends Component { closePopover={this.closePopover} panelPaddingSize="none" anchorPosition="downLeft" - ownFocus > - {this.getMenuPanel()} + {this.getMenuPanel()} ); } @@ -66,7 +66,16 @@ export class SpacesPopoverList extends Component { private getMenuPanel = () => { const { searchTerm } = this.state; - const items = this.getVisibleSpaces(searchTerm).map(this.renderSpaceMenuItem); + const options = this.getSpaceOptions(); // this.getVisibleSpaces(searchTerm).map(this.renderSpaceMenuItem); + + const noSpacesMessage = ( + + + + ); const panelProps = { className: 'spcMenu', @@ -75,16 +84,47 @@ export class SpacesPopoverList extends Component { }), }; - if (this.props.spaces.length >= SPACE_SEARCH_COUNT_THRESHOLD) { - return ( - - {this.renderSearchField()} - {this.renderSpacesListPanel(items, searchTerm)} - - ); - } - - return ; + return ( + = SPACE_SEARCH_COUNT_THRESHOLD} + searchProps={ + this.props.spaces.length >= SPACE_SEARCH_COUNT_THRESHOLD + ? ({ + placeholder: i18n.translate( + 'xpack.security.management.editRole.findSpacePlaceholder', + { + defaultMessage: 'Find a space', + } + ), + compressed: true, + } as any) + : undefined + } + noMatchesMessage={noSpacesMessage} + emptyMessage={noSpacesMessage} + options={options} + singleSelection={true} + style={{ width: 300 }} + listProps={{ + rowHeight: 40, + showIcons: false, + onFocusBadge: false, + }} + > + {(list, search) => ( + <> + + {i18n.translate('xpack.security.management.editRole.selectSpacesTitle', { + defaultMessage: 'Spaces', + })} + + {search} + {list} + + )} + + ); }; private onButtonClick = () => { @@ -101,107 +141,22 @@ export class SpacesPopoverList extends Component { }); }; - private getVisibleSpaces = (searchTerm: string): Space[] => { - const { spaces } = this.props; - - let filteredSpaces = spaces; - if (searchTerm) { - filteredSpaces = spaces.filter((space) => { - const { name, description = '' } = space; - return ( - name.toLowerCase().indexOf(searchTerm) >= 0 || - description.toLowerCase().indexOf(searchTerm) >= 0 - ); - }); - } - - return filteredSpaces; - }; - - private renderSpacesListPanel = (items: JSX.Element[], searchTerm: string) => { - if (items.length === 0) { - return ( - - - - ); - } - - return ( - - ); - }; - - private renderSearchField = () => { - return ( -
- { - - } -
- ); - }; - - private onSearchKeyDown = (e: any) => { - // 9: tab - // 13: enter - // 40: arrow-down - const focusableKeyCodes = [9, 13, 40]; - - const keyCode = e.keyCode; - if (focusableKeyCodes.includes(keyCode)) { - // Allows the spaces list panel to recieve focus. This enables keyboard and screen reader navigation - this.setState({ - allowSpacesListFocus: true, - }); - } - }; - - private onSearchFocus = () => { - this.setState({ - allowSpacesListFocus: false, - }); - }; + private getSpaceOptions = (): EuiSelectableOption[] => { + const LazySpaceAvatar = memo(this.props.spacesApiUi.components.getSpaceAvatar); - private onSearch = (searchTerm: string) => { - this.setState({ - searchTerm: searchTerm.trim().toLowerCase(), + return this.props.spaces.map((space) => { + const icon = ; // wrapped in a Suspense above + + return { + 'aria-label': space.name, + 'aria-roledescription': 'space', + label: space.name, + key: space.id, + prepend: icon, + checked: undefined, + 'data-test-subj': `${space.id}-selectableSpaceItem`, + className: 'selectableSpaceItem', + }; }); }; - - private renderSpaceMenuItem = (space: Space): JSX.Element => { - const LazySpaceAvatar = memo(this.props.spacesApiUi.components.getSpaceAvatar); - const icon = ; // wrapped in a Suspense above - return ( - - {space.name} - - ); - }; } diff --git a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx index a611457129127d..0ddca557fb40f7 100644 --- a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx +++ b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx @@ -7,24 +7,25 @@ import './spaces_menu.scss'; -import { EuiAvatar, EuiPopoverFooter, EuiPopoverTitle, EuiSelectable, EuiText } from '@elastic/eui'; +import { EuiPopoverFooter, EuiPopoverTitle, EuiSelectable, EuiText } from '@elastic/eui'; import type { EuiSelectableOption } from '@elastic/eui/src/components/selectable'; import type { EuiSelectableOnChangeEvent } from '@elastic/eui/src/components/selectable/selectable'; -import React, { Component } from 'react'; +// import type { EuiSelectableSearchProps } from '@elastic/eui/src/components/selectable/selectable_search'; +import React, { Component, lazy } from 'react'; import type { ApplicationStart, Capabilities } from '@kbn/core/public'; +import { i18n } from '@kbn/i18n'; import type { InjectedIntl } from '@kbn/i18n-react'; import { FormattedMessage, injectI18n } from '@kbn/i18n-react'; import type { Space } from '../../../common'; import { addSpaceIdToPath, ENTER_SPACE_PATH, SPACE_SEARCH_COUNT_THRESHOLD } from '../../../common'; -// import { getSpaceAvatarComponent } from '../../space_avatar'; +import { getSpaceAvatarComponent } from '../../space_avatar'; import { ManageSpacesButton } from './manage_spaces_button'; -// No need to wrap LazySpaceAvatar in an error boundary, because it is one of the first chunks loaded when opening Kibana. -// const LazySpaceAvatar = lazy(() => -// getSpaceAvatarComponent().then((component) => ({ default: component })) -// ); +const LazySpaceAvatar = lazy(() => + getSpaceAvatarComponent().then((component) => ({ default: component })) +); interface Props { id: string; @@ -56,10 +57,10 @@ class SpacesMenuUI extends Component { const spaceMenuOptions: EuiSelectableOption[] = this.getSpaceOptions(); - const noMatchesMessage = ( + const noSpacesMessage = ( @@ -79,54 +80,39 @@ class SpacesMenuUI extends Component { {...panelProps} isLoading={this.props.isLoading} searchable={this.props.spaces.length >= SPACE_SEARCH_COUNT_THRESHOLD} - // ToDo: find a way to do this more robustly, i.e. not use 'any' searchProps={ this.props.spaces.length >= SPACE_SEARCH_COUNT_THRESHOLD ? ({ - placeholder: 'Find a space', + placeholder: i18n.translate( + 'xpack.spaces.navControl.spacesMenu.findSpacePlaceholder', + { + defaultMessage: 'Find a space', + } + ), compressed: true, } as any) : undefined } - noMatchesMessage={noMatchesMessage} - emptyMessage={noMatchesMessage} + noMatchesMessage={noSpacesMessage} + emptyMessage={noSpacesMessage} options={spaceMenuOptions} singleSelection={true} style={{ width: 300 }} onChange={this.spaceSelectionChange} - listProps={{ showIcons: false }} - // onChange={(newOptions, event) => { - // const selectedSpaceItem = newOptions.filter((item) => item.checked === 'on')[0]; - - // if (!!selectedSpaceItem) { - // const urlToSelectedSpace = addSpaceIdToPath( - // this.props.serverBasePath, - // selectedSpaceItem.key, // selectedSpace.id - // ENTER_SPACE_PATH - // ); - - // console.log(event); - // console.log(`**** Event Class: ${event.constructor.name}`); - // // console.log(`**** Event Type: ${event.type}`); - - // if (event.shiftKey) { - // console.log(`**** SHIFT CLICK`); - // } else if (event.ctrlKey || event.metaKey) { - // // (event instanceof MouseEvent && (event.button === 1 || event.ctrlKey)) - // console.log(`**** CTRL/CMD CLICK`); - // } else { - // console.log(`**** NORMAL CLICK`); - // } - // } - // }} - // listProps={{ - // rowHeight: 40, - // showIcons: false, - // }} + listProps={{ + rowHeight: 40, + showIcons: false, + onFocusBadge: false, + }} > {(list, search) => ( <> - {search || 'Your spaces'} + + {search || + i18n.translate('xpack.spaces.navControl.spacesMenu.selectSpacesTitle', { + defaultMessage: 'Your spaces', + })} + {list} {this.renderManageButton()} @@ -138,18 +124,11 @@ class SpacesMenuUI extends Component { private getSpaceOptions = (): EuiSelectableOption[] => { return this.props.spaces.map((space) => { return { + 'aria-label': space.name, label: space.name, key: space.id, - prepend: ( - - ), - checked: undefined, // ToDo: set 'on' if this space ID matches the one we're in? + prepend: , + checked: undefined, 'data-test-subj': `${space.id}-selectableSpaceItem`, className: 'selectableSpaceItem', }; @@ -165,27 +144,27 @@ class SpacesMenuUI extends Component { if (!!selectedSpaceItem) { const urlToSelectedSpace = addSpaceIdToPath( this.props.serverBasePath, - selectedSpaceItem.key, // selectedSpace.id + selectedSpaceItem.key, ENTER_SPACE_PATH ); // ToDo: handle options (middle click or cmd/ctrl (new tab), shift click (new window)) - console.log(`**** Event Class: ${event.constructor.name}`); - console.log(`**** Native Event: ${event.nativeEvent}`); + // console.log(`**** Event Class: ${event.constructor.name}`); + // console.log(`**** Native Event: ${event.nativeEvent}`); if (event.shiftKey) { // Open in new window, shift is given priority over other modifiers - console.log(`**** SHIFT CLICK`); + // console.log(`**** SHIFT CLICK`); window.open(urlToSelectedSpace); } else if (event.ctrlKey || event.metaKey) { // Open in new tab - either a ctrl click or middle mouse button - console.log(`**** CTRL/CMD CLICK`); + // console.log(`**** CTRL/CMD CLICK`); // window.open(urlToSelectedSpace); // ToDo: replace with new tab } else { // Force full page reload (usually not a good idea, but we need to in order to change spaces) // console.log(`**** URL: ${urlToSelectedSpace}`); - console.log(`**** NORMAL CLICK`); - // this.props.navigateToUrl(urlToSelectedSpace); + // console.log(`**** NORMAL CLICK`); + this.props.navigateToUrl(urlToSelectedSpace); } } }; diff --git a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx index 2c8171d686de06..4d4ca4f9109bea 100644 --- a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx +++ b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx @@ -6,11 +6,17 @@ */ import type { PopoverAnchorPosition } from '@elastic/eui'; -import { EuiHeaderSectionItemButton, EuiLoadingSpinner, EuiPopover } from '@elastic/eui'; +import { + EuiFocusTrap, + EuiHeaderSectionItemButton, + EuiLoadingSpinner, + EuiPopover, +} from '@elastic/eui'; import React, { Component, lazy, Suspense } from 'react'; import type { Subscription } from 'rxjs'; import type { ApplicationStart, Capabilities } from '@kbn/core/public'; +import { i18n } from '@kbn/i18n'; import type { Space } from '../../common'; import { getSpaceAvatarComponent } from '../space_avatar'; @@ -107,9 +113,8 @@ export class NavControlPopover extends Component { anchorPosition={this.props.anchorPosition} panelPaddingSize="none" repositionOnScroll - ownFocus > - {element} + {element} ); } @@ -137,7 +142,7 @@ export class NavControlPopover extends Component { const { activeSpace } = this.state; if (!activeSpace) { - return this.getButton(, 'loading'); + return this.getButton(, 'loading spaces navigation'); } return this.getButton( @@ -154,12 +159,24 @@ export class NavControlPopover extends Component { aria-controls={popoutContentId} aria-expanded={this.state.showSpaceSelector} aria-haspopup="true" - aria-label={linkTitle} + aria-label={i18n.translate('pack.spaces.navControl.popover.spacesNavigationLabel', { + defaultMessage: 'Spaces navigation', + })} + aria-describedby="spacesNavDetails" data-test-subj="spacesNavSelector" title={linkTitle} onClick={this.toggleSpaceSelector} > {linkIcon} + ); }; From 5f63340edd22b5b06579f0b228fe9a29715f26da Mon Sep 17 00:00:00 2001 From: jeramysoucy Date: Mon, 11 Jul 2022 17:42:32 +0200 Subject: [PATCH 09/29] Updated tests for spaces popover list and created more comprehensive tests for nav control popover. Eliminated tab selection of popover panel itself. Tested screen reader extensively with Chrome on Mac. Note: Safari screen reader and tabbing behavior is not consistent and differs significantly from Chrome. --- .../spaces_popover_list.test.tsx | 91 +++++++-- .../spaces_popover_list.tsx | 19 +- .../nav_control_popover.test.tsx.snap | 43 +++-- .../nav_control/components/spaces_menu.tsx | 38 ++-- .../nav_control/nav_control_popover.test.tsx | 182 ++++++++++++++++-- .../nav_control/nav_control_popover.tsx | 1 + 6 files changed, 302 insertions(+), 72 deletions(-) diff --git a/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.test.tsx b/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.test.tsx index cecf01c66d2412..9bd3ad87964412 100644 --- a/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.test.tsx +++ b/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.test.tsx @@ -7,10 +7,11 @@ import { EuiButtonEmpty, - EuiContextMenuItem, EuiContextMenuPanel, EuiFieldSearch, EuiPopover, + EuiSelectable, + EuiSelectableListItem, } from '@elastic/eui'; import { act } from '@testing-library/react'; import React from 'react'; @@ -66,27 +67,89 @@ describe('SpacesPopoverList', () => { expect(wrapper.find(EuiContextMenuPanel)).toHaveLength(0); }); - it('clicking the button renders a context menu with the provided spaces', async () => { + it('clicking the button renders an EuiSelectable menu with the provided spaces', async () => { const wrapper = await setup(mockSpaces); await act(async () => { wrapper.find(EuiButtonEmpty).simulate('click'); }); wrapper.update(); - const menu = wrapper.find(EuiContextMenuPanel); - expect(menu).toHaveLength(1); + await act(async () => { + const menu = wrapper.find(EuiSelectable); + expect(menu).toHaveLength(1); - const items = menu.find(EuiContextMenuItem); - expect(items).toHaveLength(mockSpaces.length); + const items = menu.find(EuiSelectableListItem); + expect(items).toHaveLength(mockSpaces.length); - mockSpaces.forEach((space, index) => { - const spaceAvatar = items.at(index).find(SpaceAvatarInternal); - expect(spaceAvatar.props().space).toEqual(space); + mockSpaces.forEach((space, index) => { + const spaceAvatar = items.at(index).find(SpaceAvatarInternal); + expect(spaceAvatar.props().space).toEqual(space); + }); }); }); - it('Should NOT render a search box when there is less than 8 spaces', async () => { - const wrapper = await setup(mockSpaces); + it('should render a search box when there are 8 or more spaces', async () => { + const eightSpaces = mockSpaces.concat([ + { + id: 'space-3', + name: 'Space-3', + disabledFeatures: [], + }, + { + id: 'space-4', + name: 'Space 4', + disabledFeatures: [], + }, + { + id: 'space-5', + name: 'Space 5', + disabledFeatures: [], + }, + { + id: 'space-6', + name: 'Space 6', + disabledFeatures: [], + }, + { + id: 'space-7', + name: 'Space 7', + disabledFeatures: [], + }, + ]); + const wrapper = await setup(eightSpaces); + await act(async () => { + wrapper.find(EuiButtonEmpty).simulate('click'); + }); + wrapper.update(); + + expect(wrapper.find(EuiFieldSearch)).toHaveLength(1); + }); + + it('should NOT render a search box when there are less than 8 spaces', async () => { + const sevenSpaces = mockSpaces.concat([ + { + id: 'space-3', + name: 'Space-3', + disabledFeatures: [], + }, + { + id: 'space-4', + name: 'Space 4', + disabledFeatures: [], + }, + { + id: 'space-5', + name: 'Space 5', + disabledFeatures: [], + }, + { + id: 'space-6', + name: 'Space 6', + disabledFeatures: [], + }, + ]); + + const wrapper = await setup(sevenSpaces); await act(async () => { wrapper.find(EuiButtonEmpty).simulate('click'); }); @@ -101,11 +164,11 @@ describe('SpacesPopoverList', () => { wrapper.find(EuiButtonEmpty).simulate('click'); }); wrapper.update(); - expect(wrapper.find(EuiPopover).props().isOpen).toEqual(true); - wrapper.find(EuiPopover).props().closePopover(); - + await act(async () => { + wrapper.find(EuiPopover).props().closePopover(); + }); wrapper.update(); expect(wrapper.find(EuiPopover).props().isOpen).toEqual(false); diff --git a/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx b/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx index 252014aac7c91b..df515653a1dde0 100644 --- a/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx +++ b/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx @@ -11,12 +11,13 @@ import type { EuiSelectableOption } from '@elastic/eui'; import { EuiButtonEmpty, EuiFocusTrap, + EuiLoadingSpinner, EuiPopover, EuiPopoverTitle, EuiSelectable, EuiText, } from '@elastic/eui'; -import React, { Component, memo } from 'react'; +import React, { Component, memo, Suspense } from 'react'; import { i18n } from '@kbn/i18n'; import { FormattedMessage } from '@kbn/i18n-react'; @@ -30,14 +31,12 @@ interface Props { } interface State { - searchTerm: string; allowSpacesListFocus: boolean; isPopoverOpen: boolean; } export class SpacesPopoverList extends Component { public state = { - searchTerm: '', allowSpacesListFocus: false, isPopoverOpen: false, }; @@ -57,6 +56,7 @@ export class SpacesPopoverList extends Component { closePopover={this.closePopover} panelPaddingSize="none" anchorPosition="downLeft" + ownFocus={false} > {this.getMenuPanel()} @@ -64,9 +64,7 @@ export class SpacesPopoverList extends Component { } private getMenuPanel = () => { - const { searchTerm } = this.state; - - const options = this.getSpaceOptions(); // this.getVisibleSpaces(searchTerm).map(this.renderSpaceMenuItem); + const options = this.getSpaceOptions(); const noSpacesMessage = ( @@ -130,14 +128,12 @@ export class SpacesPopoverList extends Component { private onButtonClick = () => { this.setState({ isPopoverOpen: !this.state.isPopoverOpen, - searchTerm: '', }); }; private closePopover = () => { this.setState({ isPopoverOpen: false, - searchTerm: '', }); }; @@ -145,8 +141,11 @@ export class SpacesPopoverList extends Component { const LazySpaceAvatar = memo(this.props.spacesApiUi.components.getSpaceAvatar); return this.props.spaces.map((space) => { - const icon = ; // wrapped in a Suspense above - + const icon = ( + }> + ; + + ); return { 'aria-label': space.name, 'aria-roledescription': 'space', diff --git a/x-pack/plugins/spaces/public/nav_control/__snapshots__/nav_control_popover.test.tsx.snap b/x-pack/plugins/spaces/public/nav_control/__snapshots__/nav_control_popover.test.tsx.snap index 674f5e8b37ca2e..59fd686ee70b04 100644 --- a/x-pack/plugins/spaces/public/nav_control/__snapshots__/nav_control_popover.test.tsx.snap +++ b/x-pack/plugins/spaces/public/nav_control/__snapshots__/nav_control_popover.test.tsx.snap @@ -6,16 +6,23 @@ exports[`NavControlPopover renders without crashing 1`] = ` button={ + } closePopover={[Function]} @@ -23,24 +30,26 @@ exports[`NavControlPopover renders without crashing 1`] = ` hasArrow={true} id="spcMenuPopover" isOpen={false} - ownFocus={true} + ownFocus={false} panelPaddingSize="none" repositionOnScroll={true} > - + + id="headerSpacesMenuContent" + navigateToApp={[MockFunction]} + onManageSpacesClick={[Function]} + /> + `; diff --git a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx index 0ddca557fb40f7..fab2f37f741b0d 100644 --- a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx +++ b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx @@ -7,11 +7,16 @@ import './spaces_menu.scss'; -import { EuiPopoverFooter, EuiPopoverTitle, EuiSelectable, EuiText } from '@elastic/eui'; +import { + EuiLoadingSpinner, + EuiPopoverFooter, + EuiPopoverTitle, + EuiSelectable, + EuiText, +} from '@elastic/eui'; import type { EuiSelectableOption } from '@elastic/eui/src/components/selectable'; import type { EuiSelectableOnChangeEvent } from '@elastic/eui/src/components/selectable/selectable'; -// import type { EuiSelectableSearchProps } from '@elastic/eui/src/components/selectable/selectable_search'; -import React, { Component, lazy } from 'react'; +import React, { Component, lazy, Suspense } from 'react'; import type { ApplicationStart, Capabilities } from '@kbn/core/public'; import { i18n } from '@kbn/i18n'; @@ -39,17 +44,18 @@ interface Props { navigateToUrl: ApplicationStart['navigateToUrl']; } -interface State { - searchTerm: string; - allowSpacesListFocus: boolean; -} +// interface State { +// searchTerm: string; +// allowSpacesListFocus: boolean; +// } -class SpacesMenuUI extends Component { +// class SpacesMenuUI extends Component { +class SpacesMenuUI extends Component { // ToDo: removed unused state members - public state = { - searchTerm: '', - allowSpacesListFocus: false, - }; + // public state = { + // searchTerm: '', + // allowSpacesListFocus: false, + // }; public render() { // const { intl, spaces, serverBasePath, navigateToUrl } = this.props; @@ -127,7 +133,11 @@ class SpacesMenuUI extends Component { 'aria-label': space.name, label: space.name, key: space.id, - prepend: , + prepend: ( + }> + + + ), checked: undefined, 'data-test-subj': `${space.id}-selectableSpaceItem`, className: 'selectableSpaceItem', @@ -159,7 +169,7 @@ class SpacesMenuUI extends Component { } else if (event.ctrlKey || event.metaKey) { // Open in new tab - either a ctrl click or middle mouse button // console.log(`**** CTRL/CMD CLICK`); - // window.open(urlToSelectedSpace); // ToDo: replace with new tab + window.open(urlToSelectedSpace, '_blank'); // '_blank' causes new tab } else { // Force full page reload (usually not a good idea, but we need to in order to change spaces) // console.log(`**** URL: ${urlToSelectedSpace}`); diff --git a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.test.tsx b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.test.tsx index 0d6f46d06f904f..9be4fb5a69e3ba 100644 --- a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.test.tsx +++ b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.test.tsx @@ -5,20 +5,68 @@ * 2.0. */ -import { EuiHeaderSectionItemButton } from '@elastic/eui'; -import { waitFor } from '@testing-library/react'; +import { + EuiFieldSearch, + EuiHeaderSectionItemButton, + EuiPopover, + EuiSelectable, + EuiSelectableListItem, +} from '@elastic/eui'; +import { act, waitFor } from '@testing-library/react'; import { shallow } from 'enzyme'; import React from 'react'; import * as Rx from 'rxjs'; import { mountWithIntl } from '@kbn/test-jest-helpers'; +import type { Space } from '../../common'; import { SpaceAvatarInternal } from '../space_avatar/space_avatar_internal'; import type { SpacesManager } from '../spaces_manager'; import { spacesManagerMock } from '../spaces_manager/mocks'; import { NavControlPopover } from './nav_control_popover'; +const mockSpaces = [ + { + id: 'default', + name: 'Default Space', + description: 'this is your default space', + disabledFeatures: [], + }, + { + id: 'space-1', + name: 'Space 1', + disabledFeatures: [], + }, + { + id: 'space-2', + name: 'Space 2', + disabledFeatures: [], + }, +]; + describe('NavControlPopover', () => { + async function setup(spaces: Space[]) { + const spacesManager = spacesManagerMock.create(); + spacesManager.getSpaces = jest.fn().mockResolvedValue(spaces); + + const wrapper = mountWithIntl( + + ); + + await waitFor(() => { + wrapper.update(); + }); + + return wrapper; + } + it('renders without crashing', () => { const spacesManager = spacesManagerMock.create(); @@ -37,22 +85,12 @@ describe('NavControlPopover', () => { it('renders a SpaceAvatar with the active space', async () => { const spacesManager = spacesManagerMock.create(); - spacesManager.getSpaces = jest.fn().mockResolvedValue([ - { - id: 'foo-space', - name: 'foo', - disabledFeatures: [], - }, - { - id: 'bar-space', - name: 'bar', - disabledFeatures: [], - }, - ]); + spacesManager.getSpaces = jest.fn().mockResolvedValue(mockSpaces); // @ts-ignore readonly check spacesManager.onActiveSpaceChange$ = Rx.of({ - id: 'foo-space', - name: 'foo', + id: 'default', + name: 'Default Space', + description: 'this is your default space', disabledFeatures: [], }); @@ -72,7 +110,117 @@ describe('NavControlPopover', () => { // Wait for `getSpaces` promise to resolve await waitFor(() => { wrapper.update(); - expect(wrapper.find(SpaceAvatarInternal)).toHaveLength(3); + expect(wrapper.find(SpaceAvatarInternal)).toHaveLength(mockSpaces.length + 1); // one additional avatar for the button itself + }); + }); + + it('clicking the button renders an EuiSelectable menu with the provided spaces', async () => { + const wrapper = await setup(mockSpaces); + + await act(async () => { + wrapper.find(EuiHeaderSectionItemButton).simulate('click'); + }); + wrapper.update(); + + await act(async () => { + const menu = wrapper.find(EuiSelectable); + expect(menu).toHaveLength(1); + + const items = menu.find(EuiSelectableListItem); + expect(items).toHaveLength(mockSpaces.length); + + mockSpaces.forEach((space, index) => { + const spaceAvatar = items.at(index).find(SpaceAvatarInternal); + expect(spaceAvatar.props().space).toEqual(space); + }); + }); + }); + + it('should render a search box when there are 8 or more spaces', async () => { + const eightSpaces = mockSpaces.concat([ + { + id: 'space-3', + name: 'Space-3', + disabledFeatures: [], + }, + { + id: 'space-4', + name: 'Space 4', + disabledFeatures: [], + }, + { + id: 'space-5', + name: 'Space 5', + disabledFeatures: [], + }, + { + id: 'space-6', + name: 'Space 6', + disabledFeatures: [], + }, + { + id: 'space-7', + name: 'Space 7', + disabledFeatures: [], + }, + ]); + const wrapper = await setup(eightSpaces); + + await act(async () => { + wrapper.find(EuiHeaderSectionItemButton).simulate('click'); + }); + wrapper.update(); + + expect(wrapper.find(EuiFieldSearch)).toHaveLength(1); + }); + + it('should NOT render a search box when there are less than 8 spaces', async () => { + const sevenSpaces = mockSpaces.concat([ + { + id: 'space-3', + name: 'Space-3', + disabledFeatures: [], + }, + { + id: 'space-4', + name: 'Space 4', + disabledFeatures: [], + }, + { + id: 'space-5', + name: 'Space 5', + disabledFeatures: [], + }, + { + id: 'space-6', + name: 'Space 6', + disabledFeatures: [], + }, + ]); + const wrapper = await setup(sevenSpaces); + + await act(async () => { + wrapper.find(EuiHeaderSectionItemButton).simulate('click'); + }); + wrapper.update(); + + expect(wrapper.find(EuiFieldSearch)).toHaveLength(0); + }); + + it('can close its popover', async () => { + const wrapper = await setup(mockSpaces); + + await act(async () => { + wrapper.find(EuiHeaderSectionItemButton).simulate('click'); }); + wrapper.update(); + expect(wrapper.find(EuiPopover).props().isOpen).toEqual(true); + + await act(async () => { + wrapper.find(EuiPopover).props().closePopover(); + }); + wrapper.update(); + + expect(wrapper.find(EuiPopover).props().isOpen).toEqual(false); }); }); diff --git a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx index 4d4ca4f9109bea..76a1ffb8f7c59f 100644 --- a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx +++ b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx @@ -113,6 +113,7 @@ export class NavControlPopover extends Component { anchorPosition={this.props.anchorPosition} panelPaddingSize="none" repositionOnScroll + ownFocus={false} > {element} From 66a5d1654940a005c7ef7be3cdf718609afb9b34 Mon Sep 17 00:00:00 2001 From: jeramysoucy Date: Tue, 12 Jul 2022 11:00:51 +0200 Subject: [PATCH 10/29] Adjuted focus behavior in popover. Rebased to use EUI 60.1.2 --- .../public/nav_control/components/spaces_menu.tsx | 14 +++++++------- .../public/nav_control/nav_control_popover.tsx | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx index fab2f37f741b0d..9a2c866eb168d9 100644 --- a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx +++ b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx @@ -102,7 +102,7 @@ class SpacesMenuUI extends Component { noMatchesMessage={noSpacesMessage} emptyMessage={noSpacesMessage} options={spaceMenuOptions} - singleSelection={true} + singleSelection="always" style={{ width: 300 }} onChange={this.spaceSelectionChange} listProps={{ @@ -164,17 +164,17 @@ class SpacesMenuUI extends Component { if (event.shiftKey) { // Open in new window, shift is given priority over other modifiers - // console.log(`**** SHIFT CLICK`); - window.open(urlToSelectedSpace); + console.log(`**** SHIFT CLICK`); + // window.open(urlToSelectedSpace); } else if (event.ctrlKey || event.metaKey) { // Open in new tab - either a ctrl click or middle mouse button - // console.log(`**** CTRL/CMD CLICK`); - window.open(urlToSelectedSpace, '_blank'); // '_blank' causes new tab + console.log(`**** CTRL/CMD CLICK`); + // window.open(urlToSelectedSpace, '_blank'); // '_blank' causes new tab } else { // Force full page reload (usually not a good idea, but we need to in order to change spaces) // console.log(`**** URL: ${urlToSelectedSpace}`); - // console.log(`**** NORMAL CLICK`); - this.props.navigateToUrl(urlToSelectedSpace); + console.log(`**** NORMAL CLICK`); + // this.props.navigateToUrl(urlToSelectedSpace); } } }; diff --git a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx index 76a1ffb8f7c59f..514eef0cc68ffd 100644 --- a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx +++ b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx @@ -113,9 +113,9 @@ export class NavControlPopover extends Component { anchorPosition={this.props.anchorPosition} panelPaddingSize="none" repositionOnScroll - ownFocus={false} + // ownFocus={false} > - {element} + {element} ); } From 81745b88262a51faf4900b261922fbcd63a1adb7 Mon Sep 17 00:00:00 2001 From: jeramysoucy Date: Tue, 12 Jul 2022 13:24:44 +0200 Subject: [PATCH 11/29] Fixed typo in roles spaces popover list. Keyboard navigation with options (shift, ctrl, etc.) is not yet working - event argument in selectable onChange is null for keyboard events. --- .../spaces_popover_list.tsx | 4 ++- .../nav_control_popover.test.tsx.snap | 32 +++++++++---------- .../nav_control/components/spaces_menu.tsx | 19 +++++++---- .../nav_control/nav_control_popover.tsx | 13 +++----- 4 files changed, 34 insertions(+), 34 deletions(-) diff --git a/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx b/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx index df515653a1dde0..219cc5cb58708c 100644 --- a/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx +++ b/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx @@ -96,6 +96,8 @@ export class SpacesPopoverList extends Component { } ), compressed: true, + isClearable: true, + id: 'spacesPopoverListSearch', } as any) : undefined } @@ -143,7 +145,7 @@ export class SpacesPopoverList extends Component { return this.props.spaces.map((space) => { const icon = ( }> - ; + ); return { diff --git a/x-pack/plugins/spaces/public/nav_control/__snapshots__/nav_control_popover.test.tsx.snap b/x-pack/plugins/spaces/public/nav_control/__snapshots__/nav_control_popover.test.tsx.snap index 59fd686ee70b04..2a432971a23388 100644 --- a/x-pack/plugins/spaces/public/nav_control/__snapshots__/nav_control_popover.test.tsx.snap +++ b/x-pack/plugins/spaces/public/nav_control/__snapshots__/nav_control_popover.test.tsx.snap @@ -30,26 +30,24 @@ exports[`NavControlPopover renders without crashing 1`] = ` hasArrow={true} id="spcMenuPopover" isOpen={false} - ownFocus={false} + ownFocus={true} panelPaddingSize="none" repositionOnScroll={true} > - - - + } + id="headerSpacesMenuContent" + navigateToApp={[MockFunction]} + onManageSpacesClick={[Function]} + /> `; diff --git a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx index 9a2c866eb168d9..efc6174640d597 100644 --- a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx +++ b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx @@ -96,13 +96,15 @@ class SpacesMenuUI extends Component { } ), compressed: true, + isClearable: true, + id: 'headerSpacesMenuListSearch', } as any) : undefined } noMatchesMessage={noSpacesMessage} emptyMessage={noSpacesMessage} options={spaceMenuOptions} - singleSelection="always" + singleSelection={'always'} style={{ width: 300 }} onChange={this.spaceSelectionChange} listProps={{ @@ -131,6 +133,7 @@ class SpacesMenuUI extends Component { return this.props.spaces.map((space) => { return { 'aria-label': space.name, + 'aria-roledescription': 'space', label: space.name, key: space.id, prepend: ( @@ -161,20 +164,22 @@ class SpacesMenuUI extends Component { // ToDo: handle options (middle click or cmd/ctrl (new tab), shift click (new window)) // console.log(`**** Event Class: ${event.constructor.name}`); // console.log(`**** Native Event: ${event.nativeEvent}`); + // console.log(`**** Event Type: ${event.type}`); + // console.log(`**** Event Default Prevented: ${event.defaultPrevented}`); if (event.shiftKey) { // Open in new window, shift is given priority over other modifiers - console.log(`**** SHIFT CLICK`); - // window.open(urlToSelectedSpace); + // console.log(`**** SHIFT CLICK`); + window.open(urlToSelectedSpace); } else if (event.ctrlKey || event.metaKey) { // Open in new tab - either a ctrl click or middle mouse button - console.log(`**** CTRL/CMD CLICK`); - // window.open(urlToSelectedSpace, '_blank'); // '_blank' causes new tab + // console.log(`**** CTRL/CMD CLICK`); + window.open(urlToSelectedSpace, '_blank'); // '_blank' causes new tab } else { // Force full page reload (usually not a good idea, but we need to in order to change spaces) // console.log(`**** URL: ${urlToSelectedSpace}`); - console.log(`**** NORMAL CLICK`); - // this.props.navigateToUrl(urlToSelectedSpace); + // console.log(`**** NORMAL CLICK`); + this.props.navigateToUrl(urlToSelectedSpace); } } }; diff --git a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx index 514eef0cc68ffd..b5de971c2ce4c0 100644 --- a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx +++ b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx @@ -6,12 +6,7 @@ */ import type { PopoverAnchorPosition } from '@elastic/eui'; -import { - EuiFocusTrap, - EuiHeaderSectionItemButton, - EuiLoadingSpinner, - EuiPopover, -} from '@elastic/eui'; +import { EuiHeaderSectionItemButton, EuiLoadingSpinner, EuiPopover } from '@elastic/eui'; import React, { Component, lazy, Suspense } from 'react'; import type { Subscription } from 'rxjs'; @@ -113,7 +108,7 @@ export class NavControlPopover extends Component { anchorPosition={this.props.anchorPosition} panelPaddingSize="none" repositionOnScroll - // ownFocus={false} + ownFocus={true} > {element} @@ -160,7 +155,7 @@ export class NavControlPopover extends Component { aria-controls={popoutContentId} aria-expanded={this.state.showSpaceSelector} aria-haspopup="true" - aria-label={i18n.translate('pack.spaces.navControl.popover.spacesNavigationLabel', { + aria-label={i18n.translate('xpack.spaces.navControl.popover.spacesNavigationLabel', { defaultMessage: 'Spaces navigation', })} aria-describedby="spacesNavDetails" @@ -170,7 +165,7 @@ export class NavControlPopover extends Component { > {linkIcon}

{getSpacesFeatureDescription()}

+

{props.isLoading ? getSpacesLoadingMessage() : getSpacesFeatureDescription()}
{ element = ( Date: Tue, 26 Jul 2022 07:38:26 -0400 Subject: [PATCH 26/29] Updated jest snapshot. --- .../nav_control/__snapshots__/nav_control_popover.test.tsx.snap | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/plugins/spaces/public/nav_control/__snapshots__/nav_control_popover.test.tsx.snap b/x-pack/plugins/spaces/public/nav_control/__snapshots__/nav_control_popover.test.tsx.snap index b3228f85b93369..6d99a3526fc7b9 100644 --- a/x-pack/plugins/spaces/public/nav_control/__snapshots__/nav_control_popover.test.tsx.snap +++ b/x-pack/plugins/spaces/public/nav_control/__snapshots__/nav_control_popover.test.tsx.snap @@ -46,6 +46,7 @@ exports[`NavControlPopover renders without crashing 1`] = ` } } id="headerSpacesMenuContent" + isLoading={false} navigateToApp={[MockFunction]} toggleSpaceSelector={[Function]} /> From 0ef6330c5ab2cbf67c8d87c81079a0eb0b81fd5c Mon Sep 17 00:00:00 2001 From: jeramysoucy Date: Fri, 29 Jul 2022 15:41:24 -0400 Subject: [PATCH 27/29] Addressed flaky test behavior related to space menu nav. Implemented review feedback suggestions. --- .../spaces_popover_list/spaces_popover_list.tsx | 12 ++++-------- x-pack/plugins/spaces/public/constants.ts | 11 ----------- .../nav_control/components/spaces_description.tsx | 9 +++++++-- .../public/nav_control/components/spaces_menu.tsx | 15 +++++---------- .../public/nav_control/nav_control_popover.tsx | 2 +- .../page_objects/space_selector_page.ts | 6 +++++- 6 files changed, 22 insertions(+), 33 deletions(-) diff --git a/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx b/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx index 7cc5174fd5147a..acc71203dfff1b 100644 --- a/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx +++ b/x-pack/plugins/security/public/management/roles/edit_role/spaces_popover_list/spaces_popover_list.tsx @@ -75,16 +75,12 @@ export class SpacesPopoverList extends Component { ); - const panelProps = { - className: 'spcMenu', - title: i18n.translate('xpack.security.management.editRole.spacesPopoverList.popoverTitle', { - defaultMessage: 'Spaces', - }), - }; - return ( = SPACE_SEARCH_COUNT_THRESHOLD} searchProps={ this.props.spaces.length >= SPACE_SEARCH_COUNT_THRESHOLD diff --git a/x-pack/plugins/spaces/public/constants.ts b/x-pack/plugins/spaces/public/constants.ts index a2f84bccdb33f6..64781228d4f434 100644 --- a/x-pack/plugins/spaces/public/constants.ts +++ b/x-pack/plugins/spaces/public/constants.ts @@ -19,17 +19,6 @@ export const getSpacesFeatureDescription = () => { return spacesFeatureDescription; }; -let spacesLoadingMessage: string; - -export const getSpacesLoadingMessage = () => { - if (!spacesLoadingMessage) { - spacesLoadingMessage = i18n.translate('xpack.spaces.loadingMessage', { - defaultMessage: 'Loading...', - }); - } - return spacesLoadingMessage; -}; - export const DEFAULT_OBJECT_NOUN = i18n.translate('xpack.spaces.shareToSpace.objectNoun', { defaultMessage: 'object', }); diff --git a/x-pack/plugins/spaces/public/nav_control/components/spaces_description.tsx b/x-pack/plugins/spaces/public/nav_control/components/spaces_description.tsx index 95ae881057edca..3424bac325d181 100644 --- a/x-pack/plugins/spaces/public/nav_control/components/spaces_description.tsx +++ b/x-pack/plugins/spaces/public/nav_control/components/spaces_description.tsx @@ -12,8 +12,9 @@ import type { FC } from 'react'; import React from 'react'; import type { ApplicationStart, Capabilities } from '@kbn/core/public'; +import { i18n } from '@kbn/i18n'; -import { getSpacesFeatureDescription, getSpacesLoadingMessage } from '../../constants'; +import { getSpacesFeatureDescription } from '../../constants'; import { ManageSpacesButton } from './manage_spaces_button'; interface Props { @@ -31,10 +32,14 @@ export const SpacesDescription: FC = (props: Props) => { title: 'Spaces', }; + const spacesLoadingMessage = i18n.translate('xpack.spaces.navControl.loadingMessage', { + defaultMessage: 'Loading...', + }); + return ( -

{props.isLoading ? getSpacesLoadingMessage() : getSpacesFeatureDescription()}

+

{props.isLoading ? spacesLoadingMessage : getSpacesFeatureDescription()}
{ ); - const panelProps = { - id: this.props.id, - className: 'spcMenu', - title: this.props.intl.formatMessage({ - id: 'xpack.spaces.navControl.spacesMenu.changeCurrentSpaceTitle', - defaultMessage: 'Change current space', - }), - }; - return ( <> = SPACE_SEARCH_COUNT_THRESHOLD} searchProps={ this.props.spaces.length >= SPACE_SEARCH_COUNT_THRESHOLD diff --git a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx index 3472f83daf2371..de2c6a062f21c1 100644 --- a/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx +++ b/x-pack/plugins/spaces/public/nav_control/nav_control_popover.tsx @@ -109,7 +109,7 @@ export class NavControlPopover extends Component { anchorPosition={this.props.anchorPosition} panelPaddingSize="none" repositionOnScroll - ownFocus={true} + ownFocus > {element} diff --git a/x-pack/test/functional/page_objects/space_selector_page.ts b/x-pack/test/functional/page_objects/space_selector_page.ts index b84b22e205fea3..5aeaf49b40e166 100644 --- a/x-pack/test/functional/page_objects/space_selector_page.ts +++ b/x-pack/test/functional/page_objects/space_selector_page.ts @@ -198,7 +198,11 @@ export class SpaceSelectorPageObject extends FtrService { } async goToSpecificSpace(spaceName: string) { - await this.testSubjects.click(`${spaceName}-selectableSpaceItem`); + return await this.retry.try(async () => { + this.log.info(`SpaceSelectorPage:goToSpecificSpace(${spaceName})`); + await this.testSubjects.click(`${spaceName}-selectableSpaceItem`); + await this.common.sleep(1000); + }); } async clickSpaceAvatar(spaceId: string) { From 77fab8b503fbb656262f8c92274ec1db3e2e0bbe Mon Sep 17 00:00:00 2001 From: Jeramy Soucy Date: Mon, 1 Aug 2022 15:46:18 -0400 Subject: [PATCH 28/29] Adds sleep to functional test UI interactions. --- .../test/functional/apps/spaces/spaces_selection.ts | 6 +++--- .../functional/page_objects/space_selector_page.ts | 11 ++++++++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/x-pack/test/functional/apps/spaces/spaces_selection.ts b/x-pack/test/functional/apps/spaces/spaces_selection.ts index ea9c0db14c6a1d..4b45524defb887 100644 --- a/x-pack/test/functional/apps/spaces/spaces_selection.ts +++ b/x-pack/test/functional/apps/spaces/spaces_selection.ts @@ -84,9 +84,9 @@ export default function spaceSelectorFunctionalTests({ await PageObjects.spaceSelector.goToSpecificSpace(space5Id); await PageObjects.spaceSelector.expectHomePage(space5Id); - await PageObjects.spaceSelector.openSpacesNav(); - await PageObjects.spaceSelector.goToSpecificSpace(defaultSpaceId); - await PageObjects.spaceSelector.expectHomePage(defaultSpaceId); + // await PageObjects.spaceSelector.openSpacesNav(); + // await PageObjects.spaceSelector.goToSpecificSpace(defaultSpaceId); + // await PageObjects.spaceSelector.expectHomePage(defaultSpaceId); await PageObjects.spaceSelector.openSpacesNav(); await PageObjects.spaceSelector.goToSpecificSpace(anotherSpaceId); diff --git a/x-pack/test/functional/page_objects/space_selector_page.ts b/x-pack/test/functional/page_objects/space_selector_page.ts index 5aeaf49b40e166..18f8a933166f0f 100644 --- a/x-pack/test/functional/page_objects/space_selector_page.ts +++ b/x-pack/test/functional/page_objects/space_selector_page.ts @@ -44,6 +44,7 @@ export class SpaceSelectorPageObject extends FtrService { } else { expect(url).to.contain(`/s/${spaceId}${route}`); } + await this.common.sleep(1000); }); } @@ -58,6 +59,7 @@ export class SpaceSelectorPageObject extends FtrService { } else { expect(url).to.contain(`/s/${spaceId}`); } + await this.common.sleep(1000); }); } @@ -66,6 +68,7 @@ export class SpaceSelectorPageObject extends FtrService { return await this.retry.try(async () => { await this.testSubjects.click('spacesNavSelector'); await this.find.byCssSelector('#headerSpacesMenuContent'); + await this.common.sleep(1000); }); } @@ -197,10 +200,12 @@ export class SpaceSelectorPageObject extends FtrService { await this.testSubjects.click('space-avatar-space_b'); } - async goToSpecificSpace(spaceName: string) { + async goToSpecificSpace(spaceId: string) { return await this.retry.try(async () => { - this.log.info(`SpaceSelectorPage:goToSpecificSpace(${spaceName})`); - await this.testSubjects.click(`${spaceName}-selectableSpaceItem`); + this.log.info(`SpaceSelectorPage:goToSpecificSpace(${spaceId})`); + // await this.testSubjects.click(`${spaceId}-selectableSpaceItem`); + const spaceOption = await this.testSubjects.find(`${spaceId}-selectableSpaceItem`); + await spaceOption?.click(); await this.common.sleep(1000); }); } From 712798eaa7b0cb2fee5a51c95066649606fface2 Mon Sep 17 00:00:00 2001 From: Jeramy Soucy Date: Mon, 1 Aug 2022 17:15:32 -0400 Subject: [PATCH 29/29] Replaced use of any with ExclusiveUnion for search props of EuiSelectable. Resolved flaky test for spaces nav. --- .../nav_control/components/spaces_menu.tsx | 47 ++++++++++++------- .../apps/spaces/spaces_selection.ts | 8 ++-- .../page_objects/space_selector_page.ts | 4 +- 3 files changed, 35 insertions(+), 24 deletions(-) diff --git a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx index b60a0f0e2ae1ae..1d54c639d84d80 100644 --- a/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx +++ b/x-pack/plugins/spaces/public/nav_control/components/spaces_menu.tsx @@ -7,6 +7,7 @@ import './spaces_menu.scss'; +import type { ExclusiveUnion } from '@elastic/eui'; import { EuiLoadingSpinner, EuiPopoverFooter, @@ -15,7 +16,10 @@ import { EuiText, } from '@elastic/eui'; import type { EuiSelectableOption } from '@elastic/eui/src/components/selectable'; -import type { EuiSelectableOnChangeEvent } from '@elastic/eui/src/components/selectable/selectable'; +import type { + EuiSelectableOnChangeEvent, + EuiSelectableSearchableSearchProps, +} from '@elastic/eui/src/components/selectable/selectable'; import React, { Component, lazy, Suspense } from 'react'; import type { ApplicationStart, Capabilities } from '@kbn/core/public'; @@ -56,6 +60,30 @@ class SpacesMenuUI extends Component { ); + // In the future this could be replaced by EuiSelectableSearchableProps, but at this time is is not exported from EUI + const searchableProps: ExclusiveUnion< + { searchable: true; searchProps: EuiSelectableSearchableSearchProps<{}> }, + { searchable: false } + > = + this.props.spaces.length >= SPACE_SEARCH_COUNT_THRESHOLD + ? { + searchable: true, + searchProps: { + placeholder: i18n.translate( + 'xpack.spaces.navControl.spacesMenu.findSpacePlaceholder', + { + defaultMessage: 'Find a space', + } + ), + compressed: true, + isClearable: true, + id: 'headerSpacesMenuListSearch', + }, + } + : { + searchable: false, + }; + return ( <> { title={i18n.translate('xpack.spaces.navControl.spacesMenu.changeCurrentSpaceTitle', { defaultMessage: 'Change current space', })} - searchable={this.props.spaces.length >= SPACE_SEARCH_COUNT_THRESHOLD} - searchProps={ - this.props.spaces.length >= SPACE_SEARCH_COUNT_THRESHOLD - ? ({ - placeholder: i18n.translate( - 'xpack.spaces.navControl.spacesMenu.findSpacePlaceholder', - { - defaultMessage: 'Find a space', - } - ), - compressed: true, - isClearable: true, - id: 'headerSpacesMenuListSearch', - } as any) - : undefined - } + {...searchableProps} noMatchesMessage={noSpacesMessage} options={spaceOptions} singleSelection={'always'} diff --git a/x-pack/test/functional/apps/spaces/spaces_selection.ts b/x-pack/test/functional/apps/spaces/spaces_selection.ts index 4b45524defb887..65fa1c9f00146a 100644 --- a/x-pack/test/functional/apps/spaces/spaces_selection.ts +++ b/x-pack/test/functional/apps/spaces/spaces_selection.ts @@ -84,13 +84,13 @@ export default function spaceSelectorFunctionalTests({ await PageObjects.spaceSelector.goToSpecificSpace(space5Id); await PageObjects.spaceSelector.expectHomePage(space5Id); - // await PageObjects.spaceSelector.openSpacesNav(); - // await PageObjects.spaceSelector.goToSpecificSpace(defaultSpaceId); - // await PageObjects.spaceSelector.expectHomePage(defaultSpaceId); - await PageObjects.spaceSelector.openSpacesNav(); await PageObjects.spaceSelector.goToSpecificSpace(anotherSpaceId); await PageObjects.spaceSelector.expectHomePage(anotherSpaceId); + + await PageObjects.spaceSelector.openSpacesNav(); + await PageObjects.spaceSelector.goToSpecificSpace(defaultSpaceId); + await PageObjects.spaceSelector.expectHomePage(defaultSpaceId); }); }); diff --git a/x-pack/test/functional/page_objects/space_selector_page.ts b/x-pack/test/functional/page_objects/space_selector_page.ts index 18f8a933166f0f..26ea65e26a9cd6 100644 --- a/x-pack/test/functional/page_objects/space_selector_page.ts +++ b/x-pack/test/functional/page_objects/space_selector_page.ts @@ -203,9 +203,7 @@ export class SpaceSelectorPageObject extends FtrService { async goToSpecificSpace(spaceId: string) { return await this.retry.try(async () => { this.log.info(`SpaceSelectorPage:goToSpecificSpace(${spaceId})`); - // await this.testSubjects.click(`${spaceId}-selectableSpaceItem`); - const spaceOption = await this.testSubjects.find(`${spaceId}-selectableSpaceItem`); - await spaceOption?.click(); + await this.testSubjects.click(`${spaceId}-selectableSpaceItem`); await this.common.sleep(1000); }); }