-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Update text and icons to align with Cloud #86394
Changes from all commits
15f29ae
87606d4
a3dad89
413d26c
aa98142
8953fcc
9443c6e
1ebd481
a896248
27858f6
12eb45b
3b478c9
eb3e15b
b3b8872
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ export interface UserMenuLink { | |
iconType: IconType; | ||
href: string; | ||
order?: number; | ||
setAsProfile?: boolean; | ||
} | ||
|
||
interface Props { | ||
|
@@ -123,35 +124,39 @@ export class SecurityNavControl extends Component<Props, State> { | |
const isAnonymousUser = authenticatedUser?.authentication_provider.type === 'anonymous'; | ||
const items: EuiContextMenuPanelItemDescriptor[] = []; | ||
|
||
if (userMenuLinks.length) { | ||
const userMenuLinkMenuItems = userMenuLinks | ||
.sort(({ order: orderA = Infinity }, { order: orderB = Infinity }) => orderA - orderB) | ||
.map(({ label, iconType, href }: UserMenuLink) => ({ | ||
name: <EuiText>{label}</EuiText>, | ||
icon: <EuiIcon type={iconType} size="m" />, | ||
href, | ||
'data-test-subj': `userMenuLink__${label}`, | ||
})); | ||
items.push(...userMenuLinkMenuItems); | ||
} | ||
|
||
if (!isAnonymousUser) { | ||
const hasCustomProfileLinks = userMenuLinks.some(({ setAsProfile }) => setAsProfile === true); | ||
const profileMenuItem = { | ||
name: ( | ||
<FormattedMessage | ||
id="xpack.security.navControlComponent.editProfileLinkText" | ||
defaultMessage="Profile" | ||
defaultMessage="{profileOverridden, select, true{Preferences} other{Profile}}" | ||
values={{ profileOverridden: hasCustomProfileLinks }} | ||
/> | ||
), | ||
icon: <EuiIcon type="user" size="m" />, | ||
icon: <EuiIcon type={hasCustomProfileLinks ? 'controlsHorizontal' : 'user'} size="m" />, | ||
href: editProfileUrl, | ||
'data-test-subj': 'profileLink', | ||
}; | ||
items.push(profileMenuItem); | ||
} | ||
|
||
if (userMenuLinks.length) { | ||
const userMenuLinkMenuItems = userMenuLinks | ||
.sort(({ order: orderA = Infinity }, { order: orderB = Infinity }) => orderA - orderB) | ||
.map(({ label, iconType, href }: UserMenuLink) => ({ | ||
name: <EuiText>{label}</EuiText>, | ||
icon: <EuiIcon type={iconType} size="m" />, | ||
href, | ||
'data-test-subj': `userMenuLink__${label}`, | ||
})); | ||
|
||
items.push(...userMenuLinkMenuItems, { | ||
isSeparator: true, | ||
key: 'securityNavControlComponent__userMenuLinksSeparator', | ||
}); | ||
// Set this as the first link if there is no user-defined profile link | ||
if (!hasCustomProfileLinks) { | ||
items.unshift(profileMenuItem); | ||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: would you mind adding a simple unit test for this new behavior? |
||
items.push(profileMenuItem); | ||
} | ||
} | ||
|
||
const logoutMenuItem = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,6 +77,23 @@ export class SecurityNavControlService { | |
this.userMenuLinks$.pipe(map(this.sortUserMenuLinks), takeUntil(this.stop$)), | ||
addUserMenuLinks: (userMenuLinks: UserMenuLink[]) => { | ||
const currentLinks = this.userMenuLinks$.value; | ||
const hasCustomProfileLink = currentLinks.find(({ setAsProfile }) => setAsProfile === true); | ||
const passedCustomProfileLinkCount = userMenuLinks.filter( | ||
({ setAsProfile }) => setAsProfile === true | ||
).length; | ||
|
||
if (hasCustomProfileLink && passedCustomProfileLinkCount > 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: how do you feel about adding two simple unit tests for for these new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like that is a good idea, however I do not have experience writing them 😬 A few options:
Thanks for your feedback. It has been very helpful! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries at all!
We have a couple of tests for 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in b3b8872 |
||
throw new Error( | ||
`Only one custom profile link can be set. A custom profile link named ${hasCustomProfileLink.label} (${hasCustomProfileLink.href}) already exists` | ||
); | ||
} | ||
|
||
if (passedCustomProfileLinkCount > 1) { | ||
throw new Error( | ||
`Only one custom profile link can be passed at a time (found ${passedCustomProfileLinkCount})` | ||
); | ||
} | ||
|
||
const newLinks = [...currentLinks, ...userMenuLinks]; | ||
this.userMenuLinks$.next(newLinks); | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theirorder
is lower (assuming we set some defaultorder
for default profile link) or what is the plan?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait! I see that the cloud links are defining an
order
, but its only being used to sort within custom links.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, makes sense to me 👍