Update text and icons to align with Cloud #86394

ryankeairns · 2020-12-17T22:07:21Z

Summary

Updates the Kibana user menu links and icons to align with Cloud, as described in the related issue above.

For the sake of clarity, this PR is a small iteration on the existing menu labels, as we want to avoid ever having more than one link displaying the term 'Profile' [1]. A desired outcome of the larger, unified user profile effort seeks to avoid redundancy of user settings between a Kibana instance and Cloud. The contents of the current Kibana Account Management page, including how they will be impacted in the Cloud use case, are being hammered out in the User Profile/Settings project.

[1] While there is existing support for displaying Cloud links in the Kibana menu, these links are never displayed since the links are not currently being set in the kibana.yml file. Once those are provided by Cloud, the links will 'turn on'.

Specifically, this PR...

Adds a new setAsProfile prop on the nav control component, allowing plugin authors to set a superseding Profile link
When a superseding profile link is set, switches the label from Profile to Preferences
Displays a warning message in the console when more than one superseding profile link is defined (this is admittedly quite basic; I'm sorta beyond my depth here on how to handle this otherwise)
In addition to changing the label, switches the icon from user to controlsHorizontal (fwiw, gear is already in use for Account & Billing)
Prepends Settings for to the page title on the Account Management page

In the Cloud plugin...

Applies the new setAsProfile prop
Changes the existing Cloud profile label to Profile (only displays when Cloud enabled)
Changes the existing icon from logoCloud to user (only displays when Cloud enabled)

Menu when Kibana is self-managed

The Profile link opens the Account Management page in Kibana.

Menu when Kibana is hosted on Cloud

The Profile link takes you to the Cloud profile page.
The Preferences link opens the Account Management page in Kibana.

Current menu in the Cloud UI, for comparison

Design notes

For better alignment, we will need a few minor design adjustments on the Cloud side, as well. These suggestions largely align to the default EuiContextMenu styles:

Change icon color to the darker, default color
Remove the light color shade from the title area
Match the padding
Insure that the menu opens down as opposed to left (add the EuiPopover prop buffer={0})

Revised page title for additional clarification

To test

# Enable cloud plugin and add links
xpack.cloud.id: 'eastus2.azure.elastic-cloud.com:9243$59ef636c6917463db140321484d63cfa$a8b109c08adc43279ef48f29af1a3911'
xpack.cloud.resetPasswordUrl: 'https://cloud.elastic.co/user/settings' # Cloud profile link
xpack.cloud.accountUrl: 'https://cloud.elastic.co/account/activity' # Account link

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

ryankeairns · 2021-02-24T21:06:45Z

@elasticmachine merge upstream

kibanamachine · 2021-02-24T21:06:47Z

merge conflict between base and head

azasypkin · 2021-02-25T08:25:36Z

ACK: reviewing...

azasypkin

The approach looks good to me, but left a few questions and suggestions.

x-pack/plugins/security/public/account_management/account_management_page.tsx

x-pack/plugins/security/public/account_management/account_management_page.test.tsx

x-pack/plugins/security/public/nav_control/nav_control_component.tsx

azasypkin · 2021-02-25T08:53:34Z

x-pack/plugins/security/public/nav_control/nav_control_component.tsx

@@ -122,37 +146,38 @@ export class SecurityNavControl extends Component<Props, State> {
    const isAnonymousUser = authenticatedUser?.authentication_provider.type === 'anonymous';
    const items: EuiContextMenuPanelItemDescriptor[] = [];

+    if (userMenuLinks.length) {


question: I see we moved profile link to the bottom now, do we really want to always display Profile link after any custom user links? I thought it should be the first by default and pushed down by the custom links only if their order is lower (assuming we set some default order for default profile link) or what is the plan?

That's a great observation. The most immediate need is alignment with Cloud which means placing their link atop the list and the location of 'Preferences' (after custom links) likewise aligns neatly with their current order.

This is an admittedly simplistic approach, to start - yet it meets the objective - and I would like to see further capabilities such as an order added, subsequently. I personally don't feel comfortable adding that logic, so would you be ok with tracking this separately and handling it post-merge/as the need arises?

Oh wait! I see that the cloud links are defining an order, but its only being used to sort within custom links.

With this latest round of updates, I decided to go with simply adding the default profile link in either the first (if no other custom links are set as profile) or last position. The last position being coupled with changing the label to Preferences and changing the icon.

We could get fancier here and so something like set an order value on the default profile link, but I'm not sure if/when we'd need that, thus the simpler approach.

We could get fancier here and so something like set an order value on the default profile link, but I'm not sure if/when we'd need that, thus the simpler approach.

Agree, makes sense to me 👍

azasypkin · 2021-02-25T09:14:11Z

x-pack/plugins/security/public/nav_control/nav_control_component.tsx

@@ -66,6 +69,27 @@ export class SecurityNavControl extends Component<Props, State> {
  componentDidMount() {
    this.subscription = this.props.userMenuLinks$.subscribe(async (userMenuLinks) => {
      this.setState({ userMenuLinks });
+
+      if (userMenuLinks.length) {


question: do we really need to alter state here? Can't we just check if we have a link with setAsProfile right before we render default profile link in render? The perf impact seems to be negligible. Something like this:

if (!isAnonymousUser) { const hasCustomProfileLinks = userMenuLinks.some(({ setAsProfile }) => setAsProfile === true); const profileMenuItem = { name: ( <FormattedMessage id="xpack.security.navControlComponent.editProfileLinkText" defaultMessage="{profileOverridden, select, true{Preferences} other{Profile}}" values={{ profileOverridden: hasCustomProfileLinks }} /> ), .....

Regarding multiple links with setAsProfile, I believe we just should throw error (optionally with detailed explanation that link with labelA is already used as a profile link) in addUserMenuLinks in nav_control_service.tsx right at the moment when something wants to add a link with setAsProfile when we already have one (the service tracks all links so it should be easy to figure out). People tend to ignore console warnings anyway 🙂

Ideally I'd rather expose dedicated methods to manage (hide/rename) built-in links like Profile and Logout, but I agree that it doesn't make sense to generalize this right now since everything is still in flux, so 👍 to the setAsProfile approach.

I've removed the state stuff and am now throwing errors for two situations:

a custom profile link is already set (the name and href for the existing custom profile link are displayed)

there is no existing custom profile link but the plugin is passing in more than one (the total number passed is displayed).

ryankeairns · 2021-03-01T19:38:02Z

@elasticmachine merge upstream

ryankeairns · 2021-03-04T22:28:26Z

@elasticmachine merge upstream

kibanamachine · 2021-03-04T22:28:28Z

merge conflict between base and head

ryankeairns · 2021-03-09T17:43:23Z

@elasticmachine merge upstream

azasypkin

Looks great! Just few minor nits and questions before we 🚀 .

It seems CI is failing for your PR because of a new eslint rule that was added in the upstream recently, you should be able to automatically fix this with:

$ node scripts/eslint.js --fix x-pack/plugins/security/public/account_management/account_management_page.tsx

x-pack/plugins/security/public/nav_control/nav_control_service.tsx

azasypkin · 2021-03-10T13:47:05Z

x-pack/plugins/security/public/nav_control/nav_control_service.tsx

+          0
+        );
+
+        if (hasCustomProfileLink && passedCustomProfileLinkCount > 0) {


question: how do you feel about adding two simple unit tests for for these new if branches?

I feel like that is a good idea, however I do not have experience writing them 😬

A few options:

I can create a separate issue for adding them later (post-merge)

Perhaps you can point me at a simple example

I hate to ask, but if its small and you have any time, could I ask you to add them? (here or post-merge)

Thanks for your feedback. It has been very helpful!

No worries at all!

Perhaps you can point me at a simple example

I hate to ask, but if its small and you have any time, could I ask you to add them?

We have a couple of tests for nav_control_service.tsx here and for nav_control_component.tsx here. You can run them in a watch mode with node scripts/jest --watch x-pack/plugins/security/public/nav_control/.

But please don't waste too much of your time on this if you're stuck - just let me know and I'll push the tests to your branch my morning tomorrow and we'll merge it with a green CI. I'd prefer to keep the changes with the relevant tests in the same PR if possible.

Thank you for the information. If you don't mind pushing in the morning, I would be very grateful and can learn from what you add here. My afternoon is, unfortunately, eaten up by a dentist appoint or else I'd make an attempt myself :/

Done in b3b8872

azasypkin · 2021-03-10T13:55:58Z

x-pack/plugins/security/public/nav_control/nav_control_component.tsx

@@ -122,37 +146,38 @@ export class SecurityNavControl extends Component<Props, State> {
    const isAnonymousUser = authenticatedUser?.authentication_provider.type === 'anonymous';
    const items: EuiContextMenuPanelItemDescriptor[] = [];

+    if (userMenuLinks.length) {


We could get fancier here and so something like set an order value on the default profile link, but I'm not sure if/when we'd need that, thus the simpler approach.

Agree, makes sense to me 👍

azasypkin · 2021-03-10T14:14:48Z

x-pack/plugins/security/public/nav_control/nav_control_component.tsx

+      // Set this as the first link if there is no user-defined profile link
+      if (!hasCustomProfileLinks) {
+        items.unshift(profileMenuItem);
+      } else {


question: would you mind adding a simple unit test for this new behavior?

kibanamachine · 2021-03-11T10:38:27Z

💚 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
`security`	724.3KB	724.6KB	+290.0B

Page load bundle

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

id	before	after	diff
`cloud`	5.2KB	5.2KB	+7.0B
`security`	131.0KB	131.8KB	+812.0B
total			+819.0B

History

💚 Build #112528 succeeded 3b478c9
💔 Build #112174 failed 12eb45b
💔 Build #111245 failed 27858f6
💚 Build #110053 succeeded a896248
💚 Build #110033 succeeded 1ebd481

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

azasypkin

LGTM, thanks!

ryankeairns · 2021-03-11T14:54:09Z

Many thanks @azasypkin !!

* Update text and icons to align with Cloud * Update test to reflect new page title prefix * Change links conditionally * Simplify profile link logic * Add setAsProfile prop for overriding default link * Address feedback * remove translations since message has changed * Tidying up * Add unit tests. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

* Update text and icons to align with Cloud * Update test to reflect new page title prefix * Change links conditionally * Simplify profile link logic * Add setAsProfile prop for overriding default link * Address feedback * remove translations since message has changed * Tidying up * Add unit tests. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

Update text and icons to align with Cloud

15f29ae

ryankeairns added release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Dec 17, 2020

ryankeairns added 4 commits December 17, 2020 18:41

Update test to reflect new page title prefix

87606d4

Change links conditionally

a3dad89

Simplify profile link logic

413d26c

Add setAsProfile prop for overriding default link

aa98142

Merge branch 'master' into rk/82755-profile-links

8953fcc

ryankeairns marked this pull request as ready for review February 25, 2021 03:13

ryankeairns requested review from a team as code owners February 25, 2021 03:13

ryankeairns requested a review from gjones February 25, 2021 03:13

pgayvallet approved these changes Feb 25, 2021

View reviewed changes

azasypkin self-requested a review February 25, 2021 08:25

azasypkin reviewed Feb 25, 2021

View reviewed changes

ryankeairns added 2 commits March 1, 2021 08:38

Address feedback

9443c6e

remove translations since message has changed

1ebd481

Merge branch 'master' into rk/82755-profile-links

a896248

ryankeairns added v7.13.0 v8.0.0 and removed v7.12.0 labels Mar 1, 2021

ryankeairns requested a review from azasypkin March 2, 2021 19:51

ryankeairns mentioned this pull request Mar 3, 2021

Update Cloud plugin to handle new URLs #93436

Closed

Merge branch 'master' into rk/82755-profile-links

27858f6

Merge branch 'master' into rk/82755-profile-links

12eb45b

alexfrancoeur mentioned this pull request Mar 9, 2021

[Meta] Getting Started MVP - Onboarding #94207

Closed

13 tasks

azasypkin reviewed Mar 10, 2021

View reviewed changes

ryankeairns and others added 3 commits March 10, 2021 10:47

Tidying up

3b478c9

Merge branch 'master' into rk/82755-profile-links

eb3e15b

Add unit tests.

b3b8872

azasypkin approved these changes Mar 11, 2021

View reviewed changes

ryankeairns merged commit f61657c into elastic:master Mar 11, 2021

ryankeairns mentioned this pull request Mar 11, 2021

[7.x] Update text and icons to align with Cloud (#86394) #94436

Merged

alexfrancoeur mentioned this pull request Apr 15, 2021

Adding RBAC to new Cloud links #97308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update text and icons to align with Cloud #86394

Update text and icons to align with Cloud #86394

ryankeairns commented Dec 17, 2020 •

edited

Loading

ryankeairns commented Feb 24, 2021

kibanamachine commented Feb 24, 2021

azasypkin commented Feb 25, 2021

azasypkin left a comment

azasypkin Feb 25, 2021

ryankeairns Feb 25, 2021

ryankeairns Feb 25, 2021 •

edited

Loading

ryankeairns Mar 1, 2021

azasypkin Mar 10, 2021

azasypkin Feb 25, 2021

ryankeairns Mar 1, 2021 •

edited

Loading

ryankeairns commented Mar 1, 2021

ryankeairns commented Mar 4, 2021

kibanamachine commented Mar 4, 2021

ryankeairns commented Mar 9, 2021

azasypkin left a comment •

edited

Loading

azasypkin Mar 10, 2021

ryankeairns Mar 10, 2021

azasypkin Mar 10, 2021

ryankeairns Mar 10, 2021

azasypkin Mar 11, 2021

azasypkin Mar 10, 2021

azasypkin Mar 10, 2021

kibanamachine commented Mar 11, 2021

azasypkin left a comment

ryankeairns commented Mar 11, 2021

Update text and icons to align with Cloud #86394

Update text and icons to align with Cloud #86394

Conversation

ryankeairns commented Dec 17, 2020 • edited Loading

Summary

Specifically, this PR...

Menu when Kibana is self-managed

Menu when Kibana is hosted on Cloud

Current menu in the Cloud UI, for comparison

Design notes

Revised page title for additional clarification

To test

Checklist

ryankeairns commented Feb 24, 2021

kibanamachine commented Feb 24, 2021

azasypkin commented Feb 25, 2021

azasypkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryankeairns Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryankeairns Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

ryankeairns commented Mar 1, 2021

ryankeairns commented Mar 4, 2021

kibanamachine commented Mar 4, 2021

ryankeairns commented Mar 9, 2021

azasypkin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Mar 11, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

azasypkin left a comment

Choose a reason for hiding this comment

ryankeairns commented Mar 11, 2021

ryankeairns commented Dec 17, 2020 •

edited

Loading

ryankeairns Feb 25, 2021 •

edited

Loading

ryankeairns Mar 1, 2021 •

edited

Loading

azasypkin left a comment •

edited

Loading