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

[Spaces] UX improvements to spaces grid #188261

Merged
merged 19 commits into from
Aug 1, 2024
+300 −54
Merged

[Spaces] UX improvements to spaces grid #188261

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
5e33b47
[Spaces] UX improvements to spaces grid
tsullivan Jul 12, 2024
4448f10
update space management screenshot
tsullivan Jul 12, 2024
ce2ea9a
fix jest test
tsullivan Jul 12, 2024
984fe8d
fix fn test
tsullivan Jul 12, 2024
7efc162
Merge branch 'main' into spaces/space-grid-ux-improvement-1
tsullivan Jul 15, 2024
f89bc41
fix test
tsullivan Jul 15, 2024
ab60c1d
Merge branch 'main' into spaces/space-grid-ux-improvement-1
tsullivan Jul 17, 2024
cae256c
Clean up args
tsullivan Jul 18, 2024
2ba5bac
Merge branch 'main' into spaces/space-grid-ux-improvement-1
tsullivan Jul 18, 2024
960d5c7
Merge branch 'main' into spaces/space-grid-ux-improvement-1
tsullivan Jul 20, 2024
d803f65
Merge branch 'main' into spaces/space-grid-ux-improvement-1
tsullivan Jul 22, 2024
1cc851c
Merge branch 'main' into spaces/space-grid-ux-improvement-1
tsullivan Jul 25, 2024
36ecf51
Merge branch 'main' into spaces/space-grid-ux-improvement-1
tsullivan Jul 30, 2024
1d62cb0
Apply explicit typing per code review
tsullivan Jul 31, 2024
88496b0
Merge branch 'main' into spaces/space-grid-ux-improvement-1
tsullivan Jul 31, 2024
c71ff03
Test for current badge
tsullivan Jul 31, 2024
47bd850
Test for switch action
tsullivan Jul 31, 2024
473c048
Functional test for spaces grid
tsullivan Jul 31, 2024
3f190bc
Merge branch 'main' into spaces/space-grid-ux-improvement-1
tsullivan Aug 1, 2024
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
Binary file modified docs/spaces/images/space-management.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
16 changes: 11 additions & 5 deletions x-pack/plugins/spaces/public/management/spaces_grid/spaces_grid_page.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ featuresStart.getFeatures.mockResolvedValue([
}),
]);

const spacesGridCommonProps = {
serverBasePath: '',
maxSpaces: 1000,
};

describe('SpacesGridPage', () => {
const getUrlForApp = (appId: string) => appId;
const history = scopedHistoryMock.create();
Expand All @@ -79,7 +84,7 @@ describe('SpacesGridPage', () => {
catalogue: {},
spaces: { manage: true },
}}
maxSpaces={1000}
{...spacesGridCommonProps}
/>
);

Expand Down Expand Up @@ -138,8 +143,8 @@ describe('SpacesGridPage', () => {
catalogue: {},
spaces: { manage: true },
}}
maxSpaces={1000}
solutionNavExperiment={Promise.resolve(true)}
{...spacesGridCommonProps}
/>
);

Expand Down Expand Up @@ -173,7 +178,7 @@ describe('SpacesGridPage', () => {
catalogue: {},
spaces: { manage: true },
}}
maxSpaces={1000}
{...spacesGridCommonProps}
/>
);

Expand Down Expand Up @@ -203,6 +208,7 @@ describe('SpacesGridPage', () => {
spaces: { manage: true },
}}
maxSpaces={1}
serverBasePath={spacesGridCommonProps.serverBasePath}
/>
);

Expand Down Expand Up @@ -236,7 +242,7 @@ describe('SpacesGridPage', () => {
catalogue: {},
spaces: { manage: true },
}}
maxSpaces={1000}
{...spacesGridCommonProps}
/>
);

Expand Down Expand Up @@ -271,7 +277,7 @@ describe('SpacesGridPage', () => {
catalogue: {},
spaces: { manage: true },
}}
maxSpaces={1000}
{...spacesGridCommonProps}
/>
);

Expand Down
170 changes: 123 additions & 47 deletions x-pack/plugins/spaces/public/management/spaces_grid/spaces_grid_page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@
* 2.0.
*/

import type { EuiBasicTableColumn } from '@elastic/eui';
import {
EuiBadge,
type EuiBasicTableColumn,
EuiButton,
EuiButtonIcon,
EuiCallOut,
EuiFlexGroup,
EuiFlexItem,
EuiInMemoryTable,
EuiLink,
EuiLoadingSpinner,
Expand All @@ -31,9 +33,9 @@ import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n-react';
import { reactRouterNavigate } from '@kbn/kibana-react-plugin/public';

import type { Space } from '../../../common';
import { addSpaceIdToPath, type Space } from '../../../common';
import { isReservedSpace } from '../../../common';
import { DEFAULT_SPACE_ID } from '../../../common/constants';
import { DEFAULT_SPACE_ID, ENTER_SPACE_PATH } from '../../../common/constants';
import { getSpacesFeatureDescription } from '../../constants';
import { getSpaceAvatarComponent } from '../../space_avatar';
import { SpaceSolutionBadge } from '../../space_solution_badge';
Expand All @@ -49,6 +51,7 @@ const LazySpaceAvatar = lazy(() =>
interface Props {
spacesManager: SpacesManager;
notifications: NotificationsStart;
serverBasePath: string;
getFeatures: FeaturesPluginStart['getFeatures'];
capabilities: Capabilities;
history: ScopedHistory;
Expand All @@ -59,6 +62,7 @@ interface Props {

interface State {
spaces: Space[];
activeSpace: Space | null;
features: KibanaFeature[];
loading: boolean;
showConfirmDeleteModal: boolean;
Expand All @@ -71,6 +75,7 @@ export class SpacesGridPage extends Component<Props, State> {
super(props);
this.state = {
spaces: [],
activeSpace: null,
features: [],
loading: true,
showConfirmDeleteModal: false,
Expand Down Expand Up @@ -133,11 +138,15 @@ export class SpacesGridPage extends Component<Props, State> {
) : undefined}
<EuiInMemoryTable
itemId={'id'}
data-test-subj="spacesListTable"
items={this.state.spaces}
tableCaption={i18n.translate('xpack.spaces.management.spacesGridPage.tableCaption', {
defaultMessage: 'Kibana spaces',
})}
rowHeader="name"
rowProps={(item) => ({
'data-test-subj': `spacesListTableRow-${item.id}`,
})}
columns={this.getColumnConfig()}
pagination={true}
sorting={true}
Expand Down Expand Up @@ -221,12 +230,18 @@ export class SpacesGridPage extends Component<Props, State> {
});

const getSpaces = spacesManager.getSpaces();
const getActiveSpace = spacesManager.getActiveSpace();

try {
const [spaces, features] = await Promise.all([getSpaces, getFeatures()]);
const [spaces, activeSpace, features] = await Promise.all([
getSpaces,
getActiveSpace,
getFeatures(),
]);
this.setState({
loading: false,
spaces,
activeSpace,
features,
});
} catch (error) {
Expand All @@ -247,11 +262,13 @@ export class SpacesGridPage extends Component<Props, State> {
field: 'initials',
name: '',
width: '50px',
render: (value: string, record: Space) => {
render: (_value: string, rowRecord) => {
return (
<Suspense fallback={<EuiLoadingSpinner />}>
<EuiLink {...reactRouterNavigate(this.props.history, this.getEditSpacePath(record))}>
<LazySpaceAvatar space={record} size="s" />
<EuiLink
{...reactRouterNavigate(this.props.history, this.getEditSpacePath(rowRecord))}
>
<LazySpaceAvatar space={rowRecord} size="s" />
</EuiLink>
</Suspense>
);
Expand All @@ -263,10 +280,26 @@ export class SpacesGridPage extends Component<Props, State> {
defaultMessage: 'Space',
}),
sortable: true,
render: (value: string, record: Space) => (
<EuiLink {...reactRouterNavigate(this.props.history, this.getEditSpacePath(record))}>
{value}
</EuiLink>
render: (value: string, rowRecord) => (
<EuiFlexGroup responsive={false} alignItems="center" gutterSize="m">
<EuiFlexItem grow={false}>
<EuiLink
{...reactRouterNavigate(this.props.history, this.getEditSpacePath(rowRecord))}
data-test-subj={`${rowRecord.id}-hyperlink`}
>
{value}
</EuiLink>
</EuiFlexItem>
{this.state.activeSpace?.name === rowRecord.name && (
<EuiFlexItem grow={false}>
<EuiBadge color="primary">
{i18n.translate('xpack.spaces.management.spacesGridPage.currentSpaceMarkerText', {
defaultMessage: 'current',
})}
</EuiBadge>
</EuiFlexItem>
)}
</EuiFlexGroup>
),
},
{
Expand All @@ -275,17 +308,19 @@ export class SpacesGridPage extends Component<Props, State> {
defaultMessage: 'Description',
}),
sortable: true,
truncateText: true,
width: '30%',
},
{
field: 'disabledFeatures',
name: i18n.translate('xpack.spaces.management.spacesGridPage.featuresColumnName', {
defaultMessage: 'Features',
defaultMessage: 'Features visible',
}),
sortable: (space: Space) => {
return getEnabledFeatures(this.state.features, space).length;
},
render: (disabledFeatures: string[], record: Space) => {
const enabledFeatureCount = getEnabledFeatures(this.state.features, record).length;
render: (_disabledFeatures: string[], rowRecord) => {
const enabledFeatureCount = getEnabledFeatures(this.state.features, rowRecord).length;
if (enabledFeatureCount === this.state.features.length) {
return (
<FormattedMessage
Expand All @@ -307,7 +342,7 @@ export class SpacesGridPage extends Component<Props, State> {
return (
<FormattedMessage
id="xpack.spaces.management.spacesGridPage.someFeaturesEnabled"
defaultMessage="{enabledFeatureCount} / {totalFeatureCount} features visible"
defaultMessage="{enabledFeatureCount} / {totalFeatureCount}"
values={{
enabledFeatureCount,
totalFeatureCount: this.state.features.length,
Expand Down Expand Up @@ -350,39 +385,80 @@ export class SpacesGridPage extends Component<Props, State> {
}),
actions: [
{
render: (record: Space) => (
<EuiButtonIcon
data-test-subj={`${record.name}-editSpace`}
aria-label={i18n.translate(
'xpack.spaces.management.spacesGridPage.editSpaceActionName',
{
defaultMessage: `Edit {spaceName}.`,
values: { spaceName: record.name },
}
)}
color={'primary'}
iconType={'pencil'}
{...reactRouterNavigate(this.props.history, this.getEditSpacePath(record))}
/>
),
isPrimary: true,
name: i18n.translate('xpack.spaces.management.spacesGridPage.editSpaceActionName', {
defaultMessage: `Edit`,
}),
description: (rowRecord) =>
i18n.translate('xpack.spaces.management.spacesGridPage.editSpaceActionDescription', {
defaultMessage: `Edit {spaceName}.`,
values: { spaceName: rowRecord.name },
}),
type: 'icon',
icon: 'pencil',
color: 'primary',
href: (rowRecord) =>
reactRouterNavigate(this.props.history, this.getEditSpacePath(rowRecord)).href,
onClick: (rowRecord) =>
reactRouterNavigate(this.props.history, this.getEditSpacePath(rowRecord)).onClick,
'data-test-subj': (rowRecord) => `${rowRecord.name}-editSpace`,
},
{
available: (record: Space) => !isReservedSpace(record),
render: (record: Space) => (
<EuiButtonIcon
data-test-subj={`${record.name}-deleteSpace`}
aria-label={i18n.translate(
'xpack.spaces.management.spacesGridPage.deleteActionName',
{
defaultMessage: `Delete {spaceName}.`,
values: { spaceName: record.name },
}
)}
color={'danger'}
iconType={'trash'}
onClick={() => this.onDeleteSpaceClick(record)}
/>
),
isPrimary: true,
name: i18n.translate('xpack.spaces.management.spacesGridPage.switchSpaceActionName', {
defaultMessage: 'Switch',
Copy link
Member Author

Choose a reason for hiding this comment

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

@kevinsweet

You brought up a point about an overall goal to enforce more intentional navigation paths. In that regard, we maybe don't want to include a "Switch" action here to switch the user to another Space. We already have trained users to use the Space Selector: the drop-down in the top nav.

I would also mention one difference between how the Space Selector works:

  • When the user changes their Space using the Space Selector, they are navigated to the "Space landing page" for that space
  • When the user changes their Space using this action, the navigation would keep them in the Spaces Management page.

I'll put this PR in Draft mode for us to discuss. I'd like to avoid having to "undo" things as we roll out the project. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally would vote for keeping the switch here too, because as Tim mentioned, it doesn't take the user away from their context or otherwise detract, given the way it's implemented here.

But also see your point Kevin. NOT having it here should not be a big problem either.

  • One more thing to consider: if we remove this option, we're left with only 2 more: edit and delete. Hence, we can remove ellipses menu as well, and save some clicks (we were there before , I know, but just not to forget about it)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll reach out to the folks involved in this project, and we'll use this comment to vote and make a decision.

👍🏻 = keep the "Switch" action
👎🏻 = remove it

Copy link
Member

Choose a reason for hiding this comment

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

Upvoted to keep the switcher. IMO, while true this is a duplication of function, the user looking at the list of spaces is operating ON that list. Whether aware or not of the "active space" concept, their focus is on the list, not the breadcrumb bar. I'll draw a comparison to the Maximize button vs. double click on a title bar. Both accomplish the same thing. While dragging a window, the focus is on the title bar. Sure the user could drop it, navigate to the maximize button, and use that, but it's a cumbersome, disjointed experience.

Also, I believe this was discussed with Ryan during the design phase prior to engineering getting started and a decision was reached. That's why it appears in the design as it does. 🙂. Unless there is a dang good reason to change the design and jeopardize the MVP implementation, let's stick to the design.

Copy link

Choose a reason for hiding this comment

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

Hmm... I don't follow. What's the point of a user being able to switch spaces and still be in space settings? It's the same screen, same menu with all the same options and controls, but now i'm in a different space in the background?

It's odd to mix these usually separate things "administration" and "navigation."

Copy link
Member

Choose a reason for hiding this comment

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

I agree it is odd to mix them, but I think our case is a bit unusual. In our case the space is sort of a non hierarchical third dimension or "layer", and since the active space is different from all the others, it just... "feels" right to be able to control it from the list of spaces. Not having it there feels like something is missing and the normal space switcher combo feels too "far" from my focus. These are just my gut opinions, but it looks like the majority of folks have the same gut.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks all, for the discussion. I believe it was worth it: got to voice concerns and listen. We have a result from the vote: the Switch action will stay.

Copy link

Choose a reason for hiding this comment

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

Nice to see this one moving fwd at pace. Bigger picture wise I'm totally on board, but in future situations, we definitely need to be able to answer the question, "what exactly is distinct purpose of a given UI element, from the user point of view?" Otherwise we're just adding ui clutter/complexity for no reason. But ack: that kind of consideration should happen earlier in the process prior to dev, for sure 👍 Onward and upward - excited for this one 🚀

}),
description: (rowRecord) =>
this.state.activeSpace?.name !== rowRecord.name
? i18n.translate(
'xpack.spaces.management.spacesGridPage.switchSpaceActionDescription',
{
defaultMessage: 'Switch to {spaceName}',
values: { spaceName: rowRecord.name },
}
)
: i18n.translate(
'xpack.spaces.management.spacesGridPage.switchSpaceActionDisabledDescription',
{
defaultMessage: '{spaceName} is the current space',
values: { spaceName: rowRecord.name },
}
),
type: 'icon',
icon: 'merge',
color: 'primary',
href: (rowRecord) =>
addSpaceIdToPath(
this.props.serverBasePath,
rowRecord.id,
`${ENTER_SPACE_PATH}?next=/app/management/kibana/spaces/`
),
enabled: (rowRecord) => this.state.activeSpace?.name !== rowRecord.name,
'data-test-subj': (rowRecord) => `${rowRecord.name}-switchSpace`,
},
{
name: i18n.translate('xpack.spaces.management.spacesGridPage.deleteActionName', {
defaultMessage: `Delete`,
}),
description: (rowRecord) =>
isReservedSpace(rowRecord)
? i18n.translate(
'xpack.spaces.management.spacesGridPage.deleteActionDisabledDescription',
{
defaultMessage: `{spaceName} is reserved`,
values: { spaceName: rowRecord.name },
}
)
: i18n.translate('xpack.spaces.management.spacesGridPage.deleteActionDescription', {
defaultMessage: `Delete {spaceName}`,
values: { spaceName: rowRecord.name },
}),
type: 'icon',
icon: 'trash',
color: 'danger',
onClick: (rowRecord) => this.onDeleteSpaceClick(rowRecord),
enabled: (rowRecord: Space) => !isReservedSpace(rowRecord),
'data-test-subj': (rowRecord) => `${rowRecord.name}-deleteSpace`,
},
],
});
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/spaces/public/management/spaces_management_app.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ describe('spacesManagementApp', () => {
css="You have tried to stringify object returned from \`css\` function. It isn't supposed to be used directly (e.g. as value of the \`className\` prop), but rather handed to emotion so it can handle it (e.g. as value of \`css\` prop)."
data-test-subj="kbnRedirectAppLink"
>
Spaces Page: {"capabilities":{"catalogue":{},"management":{},"navLinks":{}},"notifications":{"toasts":{}},"spacesManager":{"onActiveSpaceChange$":{}},"history":{"action":"PUSH","length":1,"location":{"pathname":"/","search":"","hash":""}},"maxSpaces":1000,"solutionNavExperiment":{}}
Spaces Page: {"capabilities":{"catalogue":{},"management":{},"navLinks":{}},"notifications":{"toasts":{}},"spacesManager":{"onActiveSpaceChange$":{}},"serverBasePath":"","history":{"action":"PUSH","length":1,"location":{"pathname":"/","search":"","hash":""}},"maxSpaces":1000,"solutionNavExperiment":{}}
</div>
</div>
`);
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/spaces/public/management/spaces_management_app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export const spacesManagementApp = Object.freeze({
text: title,
href: `/`,
};
const { notifications, application, chrome } = coreStart;
const { notifications, application, chrome, http } = coreStart;

chrome.docTitle.change(title);

Expand All @@ -63,6 +63,7 @@ export const spacesManagementApp = Object.freeze({
getFeatures={features.getFeatures}
notifications={notifications}
spacesManager={spacesManager}
serverBasePath={http.basePath.serverBasePath}
history={history}
getUrlForApp={application.getUrlForApp}
maxSpaces={config.maxSpaces}
Expand Down
2 changes: 2 additions & 0 deletions x-pack/test/functional/page_objects/space_selector_page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,8 @@ export class SpaceSelectorPageObject extends FtrService {
}

async clickOnDeleteSpaceButton(spaceName: string) {
const collapsedButtonSelector = '[data-test-subj=euiCollapsedItemActionsButton]';
await this.find.clickByCssSelector(`#${spaceName}-actions ${collapsedButtonSelector}`);
await this.testSubjects.click(`${spaceName}-deleteSpace`);
}

Expand Down