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.

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

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Jul 12, 2024

Summary

This PR offers UX improvements to the Spaces Management listing page which are part of epic: https://github.com/elastic/kibana-team/issues/785

  • Use a badge to denote the current space
  • Update wording of the "features visible" column header
  • Truncate Space description text
  • Add an action to switch to the space identified by the table row.

In the Roles & Spaces UX Improvements project, our roll out plan is work in #184697 and to pull small mergeable changes a little at a time, to release the changes as separate PRs.

Screenshot

Before:
image

After:
image

Release Note

Added minor user experience improvements to Spaces Management in Stack Management.

Checklist

Delete any items that are not applicable to this PR.

@tsullivan tsullivan force-pushed the spaces/space-grid-ux-improvement-1 branch 2 times, most recently from cf7a186 to 40c3457 Compare July 12, 2024 19:49
@tsullivan tsullivan added release_note:enhancement Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! labels Jul 12, 2024
@tsullivan tsullivan force-pushed the spaces/space-grid-ux-improvement-1 branch from 40c3457 to 984fe8d Compare July 12, 2024 22:26
@elastic elastic deleted a comment from elasticmachine Jul 12, 2024
@tsullivan tsullivan marked this pull request as ready for review July 12, 2024 22:29
@tsullivan tsullivan requested a review from a team as a code owner July 12, 2024 22:29
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@ryankeairns
Copy link
Contributor

Really nice set of UX improvements ticked off with a tidy, focused PR. Love it!

@tsullivan tsullivan self-assigned this Jul 15, 2024
@jeramysoucy jeramysoucy self-requested a review July 16, 2024 07:16
Copy link
Member Author

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

Need to make a decision about the "Switch" action in the table list of Spaces.

),
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.

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.

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 🚀

@tsullivan tsullivan marked this pull request as draft July 17, 2024 01:43
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
spaces 180.6KB 182.5KB +1.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
spaces 27.5KB 27.6KB +48.0B

History

cc @tsullivan

@jeramysoucy jeramysoucy removed their request for review July 19, 2024 14:37
@tsullivan tsullivan marked this pull request as ready for review July 20, 2024 03:33
@elena-shostak elena-shostak self-assigned this Jul 30, 2024
@tsullivan tsullivan requested a review from a team as a code owner July 30, 2024 15:20
Copy link
Member

@dmlemeshko dmlemeshko left a comment

Choose a reason for hiding this comment

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

x-pack/test/functional/page_objects/space_selector_page.ts changes LGTM

@jeramysoucy jeramysoucy self-requested a review July 30, 2024 23:00
@elena-shostak elena-shostak requested review from elena-shostak and jeramysoucy and removed request for jeramysoucy July 31, 2024 08:25
Copy link
Contributor

@elena-shostak elena-shostak left a comment

Choose a reason for hiding this comment

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

Looks good!

Could we please add more tests for this enhanced UI? (current badge display, actions menu).

@jeramysoucy jeramysoucy removed their request for review July 31, 2024 12:25
@tsullivan
Copy link
Member Author

/ci

@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6667

[✅] x-pack/test/functional/apps/spaces/config.ts: 42/42 tests passed.

see run history

@tsullivan
Copy link
Member Author

new functional tests are passing in flaky test run: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6667

await this.find.clickByCssSelector(`#${spaceName}-actions ${collapsedButtonSelector}`);
// click context menu item
await this.find.clickByCssSelector(
`.euiContextMenuItem[data-test-subj="${spaceName}-switchSpace"]` // can not use testSubj: multiple elements exist with the same data-test-subj
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 couldn't see a way to click the link in the context menu simply using the data-test-subj, since multiple elements existed with the same attribute (the link text and the button). So I needed to use a more specific selector that involves the EUI class name.

@tsullivan tsullivan enabled auto-merge (squash) August 1, 2024 18:30
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #88 / Cloud Security Posture Test adding Cloud Security Posture Integrations CSPM GCP CIS_GCP Organization Post Installation Google Cloud Shell modal pops up after user clicks on Save button when adding integration, when there are no Project ID or Organization ID provided, it should use default value

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
spaces 182.5KB 184.5KB +2.0KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
spaces 29.6KB 29.7KB +48.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @tsullivan @elena-shostak

@tsullivan tsullivan merged commit 4e0910a into elastic:main Aug 1, 2024
21 checks passed
@kibanamachine kibanamachine added v8.16.0 backport:skip This commit does not require backporting labels Aug 1, 2024
@tsullivan tsullivan deleted the spaces/space-grid-ux-improvement-1 branch August 1, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:enhancement Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants