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

Adds cloud links to user popover #66825

Merged
merged 14 commits into from
Nov 6, 2020
Merged

Conversation

cqliu1
Copy link
Contributor

@cqliu1 cqliu1 commented May 15, 2020

Summary

Closes #62863.
Related to https://github.com/elastic/cloud/issues/57695.

This converts the Kibana user menu to a context menu and adds cloud links to the user menu in the global nav. This works off of the assumption that the URLs for the additional cloud links will be configured in the kibana.yml.

Non-cloud user

Screen Shot 2020-11-04 at 2 06 33 PM

Cloud user

Screen Shot 2020-11-05 at 4 20 10 PM

To test

You can mock enabling the cloud plugin locally by defining a cloud ID and providing the urls for the profile links added in this PR in your kibana.dev.yml, like so:

# Enable cloud plugin
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.

For maintainers

@cqliu1 cqliu1 added the WIP Work in progress label May 15, 2020
@ryankeairns
Copy link
Contributor

@cqliu1 what do I add to the kibana.yml file in order to test this out? or can I add it to the kibana.dev.yml file?

@cqliu1
Copy link
Contributor Author

cqliu1 commented May 28, 2020

@cqliu1 what do I add to the kibana.yml file in order to test this out? or can I add it to the kibana.dev.yml file?

Currently, the cloud links are still hard coded, so all you need to do to test this PR right now is pass a cloud ID and a cloud password reset url in your kibana.dev.yml, like this:

# Enable cloud plugin
xpack.cloud.id: 'eastus2.azure.elastic-cloud.com:9243$59ef636c6917463db140321484d63cfa$a8b109c08adc43279ef48f29af1a3911'
xpack.cloud.resetPasswordUrl: 'https://cloud.elastic.co/user/settings'

I'll provide an updated snippet with the additional links in the description up top once I have the UI reading the links from the config file.

@ryankeairns
Copy link
Contributor

Tried it and it worked!
I'll withhold an approval until out of draft mode, but this is looking great.

@cqliu1 cqliu1 force-pushed the cloud-profile-link branch from c5b69cd to b8e48d6 Compare June 1, 2020 19:11
@cqliu1 cqliu1 added enhancement New value added to drive a business result release_note:enhancement v7.9.0 v8.0.0 labels Jun 2, 2020
@cqliu1 cqliu1 force-pushed the cloud-profile-link branch from b8e48d6 to 67b24b2 Compare June 4, 2020 19:57
@ryankeairns
Copy link
Contributor

ryankeairns commented Jun 5, 2020

Catherine and I talked on the side about whether we should also display the current Kibana profile link alongside the Cloud profile/account/security links. It seems that would be necessary as it allows users to reset their kibana password.

For the time being, we've decided to leave it in and temporarily name the Cloud profile link as 'Cloud profile' (accompanied by the Cloud logo in lieu of the user icon).

This will be revisited once the Cloud design and API work is complete. At that time, we can discuss as a group and determine the final names, icons, etc.

@cqliu1 cqliu1 force-pushed the cloud-profile-link branch 4 times, most recently from 7507d54 to 14e71bd Compare June 25, 2020 21:46
@cqliu1 cqliu1 marked this pull request as ready for review June 25, 2020 23:51
@cqliu1 cqliu1 requested review from a team as code owners June 25, 2020 23:51
@cqliu1 cqliu1 added the review label Jun 25, 2020
@cqliu1 cqliu1 requested a review from ryankeairns June 25, 2020 23:51
@cqliu1 cqliu1 removed the WIP Work in progress label Jun 25, 2020
@legrego legrego self-requested a review June 26, 2020 10:01
@legrego
Copy link
Member

legrego commented Jun 26, 2020

ACK: will review on Monday

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

I wonder if it would be better to invert the dependencies here a bit and have the security plugin expose an API for adding new links to the dropdown. Advantages of this would be:

  • It's more flexible, other plugins can also add more links to this dropdown
  • We can keep all the cloud-specific bits in the Cloud plugin rather than spreading it out over the codebase

@elastic/kibana-security thoughts?

@legrego
Copy link
Member

legrego commented Jun 29, 2020

I wonder if it would be better to invert the dependencies here a bit and have the security plugin expose an API for adding new links to the dropdown. Advantages of this would be:

  • It's more flexible, other plugins can also add more links to this dropdown
  • We can keep all the cloud-specific bits in the Cloud plugin rather than spreading it out over the codebase

@elastic/kibana-security thoughts?

++ I'd rather see the cloud plugin add a dependency on the security plugin, and have the security plugin expose a service to add links to the dropdown.

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Small suggestion, code looks good.

Comment on lines 6 to 10
.securityNavControlComponent__logoutLink {
border-top: $euiBorderThin;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove this or only apply it if cloud links exist. It looks a bit odd when there's only two items.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up using the separator menu item instead of relying on CSS to add that divider. https://github.com/elastic/kibana/pull/66825/files#diff-244d6513d4e0e5f7effeaa007127777d567372db67bf0e3ae54bbea6481bd2a8R160

@cqliu1
Copy link
Contributor Author

cqliu1 commented Jun 29, 2020

@joshdover @legrego That makes a lot of sense. Let me switch this PR back into a WIP while I rework this feature and reverse the dependencies.

@cqliu1 cqliu1 removed the review label Jun 29, 2020
@cqliu1 cqliu1 marked this pull request as ready for review November 5, 2020 20:51
@cqliu1 cqliu1 requested a review from a team November 5, 2020 20:51
Copy link
Contributor

@ryankeairns ryankeairns 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! Well aligned to the Cloud UI menu (though they need to add the buffer prop as well 😉 ) and leaves us in a good spot to iron on the final links (profile vs cloud profile, etc.).

Thanks!

@@ -103,44 +183,8 @@ export class SecurityNavControl extends Component<Props, State> {
closePopover={this.closeMenu}
panelPaddingSize="none"
>
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a buffer={0} here will make the popover open below the button, regardless of the popover content. (🎩 hat tip @cchaos )

@ryankeairns ryankeairns self-requested a review November 5, 2020 21:39
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

Accidentally approved, need to add that buffer={0} prop.

@ryankeairns ryankeairns self-requested a review November 5, 2020 22:20
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
cloud 6 7 +1
security 460 469 +9
total +10

async chunks size

id before after diff
security 814.1KB 784.0KB -30.1KB

distributable file count

id before after diff
default 42729 42730 +1

page load bundle size

id before after diff
cloud 5.2KB 6.1KB +942.0B
security 151.4KB 162.3KB +10.9KB
total +11.8KB

History

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

@cqliu1 cqliu1 merged commit 8cdf566 into elastic:master Nov 6, 2020
@cqliu1 cqliu1 deleted the cloud-profile-link branch November 6, 2020 00:41
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

There's certainly more to be sorted out here. The general premise we're after is that the top header is truly global and that we don't differentiate between Cloud UI and Kibana UI. Now, there's a lot of related work that needs to happen for this to become reality, but I would refrain from separating things out. Given that, we may want to replace the Cloud logo, ultimately.

My preference would be to get this PR merged since Catherine is headed back to Canvas 😢 , and then move our discussion to #82755 . We've got time to sort this out before the Cloud side catches up at which point this 'turns on'.

@ryankeairns That works for me.


I'm surprised to find this was merged without codeowners approval?

Comment on lines +75 to +77
addUserMenuLinks: (userMenuLink: UserMenuLink[]) => {
this.userMenuLinks$.next(userMenuLink);
},
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this was in the spirit of what @pgayvallet and I were asking for. next will still replace the previously added links with whatever is provided by the subsequent caller.

The reason we asked this to be changed to addUserMenuLinks was so that multiple plugins could register menu links without clobbering the links that other plugins may have added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for merging prematurely! I got a little too excited, and I thought I addressed all of your feedback. I misunderstood the ask as just a rename. I'll go ahead and revert the commit, and open a new PR.

cqliu1 added a commit that referenced this pull request Nov 6, 2020
@cqliu1 cqliu1 added release_note:skip Skip the PR/issue when compiling release notes reverted and removed release_note:enhancement v7.11.0 labels Nov 6, 2020
@cqliu1 cqliu1 restored the cloud-profile-link branch November 6, 2020 04:00
@cqliu1 cqliu1 mentioned this pull request Nov 6, 2020
7 tasks
spalger pushed a commit that referenced this pull request Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result release_note:skip Skip the PR/issue when compiling release notes reverted review v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Render Cloud links in the Kibana user profile dropdown
7 participants