Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Accesibility: spaces] Switching to another space should show a notification #36746

Closed
wants to merge 10 commits into from
Closed

[Accesibility: spaces] Switching to another space should show a notification #36746

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a7a01c1
WIP: ...
PhilippBaranovskiy May 21, 2019
4286a11
Merge branch 'master' into 23885
PhilippBaranovskiy Jun 19, 2019
a0c204d
Added toast notification showing as well as setting a timer to run re…
PhilippBaranovskiy Jun 20, 2019
a6a3fed
Merge branch 'master' into 23885
PhilippBaranovskiy Jun 20, 2019
5e55c6c
Merge branch 'master' into 23885
PhilippBaranovskiy Jun 24, 2019
eecc027
Review updates
PhilippBaranovskiy Jun 24, 2019
ac3b7b6
Review updates
PhilippBaranovskiy Jun 24, 2019
1ab3954
Merge branch 'master' into 23885
PhilippBaranovskiy Jun 27, 2019
f6b115d
make fetching url and showing notification is paralleled
PhilippBaranovskiy Jun 27, 2019
bd7138a
made triggering invisible notification
PhilippBaranovskiy Jun 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions x-pack/plugins/spaces/public/views/nav_control/components/spaces_header_nav_button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,17 @@ import { ButtonProps } from '../types';

export class SpacesHeaderNavButton extends Component<ButtonProps> {
public render() {
const { spaceSelectorShown, linkTitle, linkIcon, toggleSpaceSelector } = this.props;
return (
<EuiHeaderSectionItemButton
aria-controls="headerSpacesMenuList"
aria-expanded={this.props.spaceSelectorShown}
aria-expanded={spaceSelectorShown}
aria-haspopup="true"
aria-label={this.props.linkTitle}
title={this.props.linkTitle}
onClick={this.props.toggleSpaceSelector}
aria-label={linkTitle}
title={linkTitle}
onClick={toggleSpaceSelector}
>
{this.props.linkIcon}
{linkIcon}
</EuiHeaderSectionItemButton>
);
}
Expand Down
16 changes: 15 additions & 1 deletion x-pack/plugins/spaces/public/views/nav_control/components/spaces_menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,15 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { EuiContextMenuItem, EuiContextMenuPanel, EuiFieldSearch, EuiText } from '@elastic/eui';
import {
EuiContextMenuItem,
EuiContextMenuPanel,
EuiFieldSearch,
EuiText,
EuiGlobalToastList,
} from '@elastic/eui';
import { FormattedMessage, InjectedIntl, injectI18n } from '@kbn/i18n/react';
import { i18n } from '@kbn/i18n';
import React, { Component } from 'react';
import { SPACE_SEARCH_COUNT_THRESHOLD } from '../../../../common/constants';
import { Space } from '../../../../common/model/space';
Expand Down Expand Up @@ -163,13 +170,20 @@ class SpacesMenuUI extends Component<Props, State> {

private renderSpaceMenuItem = (space: Space): JSX.Element => {
const icon = <SpaceAvatar space={space} size={'s'} />;
const itemAriaLabel = i18n.translate('xpack.spaces.headerNavigationSwitchAriaLabel', {
defaultMessage: '; Switch to {spaceName} space',
values: {
spaceName: space.name,
},
});
return (
<EuiContextMenuItem
key={space.id}
icon={icon}
onClick={this.props.onSelectSpace.bind(this, space)}
toolTipTitle={space.description && space.name}
toolTipContent={space.description}
aria-label={itemAriaLabel}
>
{space.name}
</EuiContextMenuItem>
Expand Down
20 changes: 18 additions & 2 deletions x-pack/plugins/spaces/public/views/nav_control/nav_control_popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { EuiAvatar, EuiPopover, PopoverAnchorPosition } from '@elastic/eui';
import React, { Component, ComponentClass } from 'react';
import { EuiAvatar, EuiPopover, PopoverAnchorPosition } from '@elastic/eui';
import { toastNotifications } from 'ui/notify';
import { i18n } from '@kbn/i18n';
import { Space } from '../../../common/model/space';
import { SpaceAvatar } from '../../components';
import { SpacesManager } from '../../lib/spaces_manager';
Expand Down Expand Up @@ -159,6 +161,20 @@ export class NavControlPopover extends Component<Props, State> {
};

private onSelectSpace = (space: Space) => {
this.props.spacesManager.changeSelectedSpace(space);
toastNotifications.add({
title: i18n.translate(
'xpack.apm.serviceDetails.enableAnomalyDetectionPanel.jobCreationFailedNotificationTitle',
{
defaultMessage: 'Switching to {spaceName} space',
values: {
spaceName: space.name,
},
}
),
onClose: () => this.props.spacesManager.changeSelectedSpace(space),
});

setTimeout(() => this.props.spacesManager.changeSelectedSpace(space), 4000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching between spaces today already takes a decent amount of time because of the full page reload that's required, and the fact that we place users on the Kibana home screen following a space switch. So it likely takes users at least 10 seconds to switch spaces and navigate to the application they're interested in.

I feel that adding an additional 4 seconds before any of that happens would unnecessarily frustrate a large number of users. Now that the space selector menu has a descriptive aria-label ("Switch to {spaceName} space"), do we need this additional notification? We don't do this dual-notification when switching between applications, for example.

IF we do need this notification, then the toast is rather hard to discover. This is perhaps more of a question for the design team, but switching a space happens in the top left of the screen, but the notification appears in the bottom right of the screen:
image

Copy link
Contributor Author

@PhilippBaranovskiy PhilippBaranovskiy Jun 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legrego this is not going to be visible in the future, however, making toast notifications able to hidden but announced is another PR: elastic/eui#2055

Delay you mentioned is crucial indeed.
I think you are right that we have already aria-label before hit switching, so the main deal here is to confirm for a user that switching process is starting.
Would it be enough to announce "Switching..." and reload the page? What do you think?

return true;
};
}