Skip to content

[dashboard] avatar menu #3491

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

Merged
merged 1 commit into from
Mar 19, 2021
Merged

Conversation

svenefftinge
Copy link
Member

@svenefftinge svenefftinge commented Mar 18, 2021

fixes #3488

@svenefftinge svenefftinge force-pushed the se/dashboard-avatar-menu branch 3 times, most recently from b7be278 to 897b5b1 Compare March 18, 2021 16:27
@svenefftinge svenefftinge marked this pull request as ready for review March 18, 2021 16:58
@svenefftinge svenefftinge force-pushed the se/dashboard-avatar-menu branch 2 times, most recently from 6fb62a7 to 3f6a5dd Compare March 18, 2021 17:09
@gtsiolis gtsiolis self-assigned this Mar 19, 2021
@gtsiolis
Copy link
Contributor

gtsiolis commented Mar 19, 2021

/werft run

👍 started the job as gitpod-build-se-dashboard-avatar-menu.7

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Je vous remercie! LGTM. Left some minor comments. 💬

@@ -50,12 +51,16 @@ function ContextMenu(props: ContextMenuProps) {
{props.children}
</div>
{expanded?
<div className={`z-50 ${props.width || 'w-40'} bg-white absolute py-2 right-0 flex flex-col border border-gray-200 rounded-lg space-y-2`}>
<div className={`mt-2 z-50 ${props.width || 'w-40'} bg-white absolute right-0 flex flex-col border border-gray-200 rounded-lg`}>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Let's change the dropdown width to w-48 as with the more actions button (see #3496 (review)).

Suggested change
<div className={`mt-2 z-50 ${props.width || 'w-40'} bg-white absolute right-0 flex flex-col border border-gray-200 rounded-lg`}>
<div className={`mt-2 z-50 ${props.width || 'w-48'} bg-white absolute right-0 flex flex-col border border-gray-200 rounded-lg`}>

<img className="rounded-full w-6 h-6"
src={user?.avatarUrl || ''} alt={user?.name || 'Anonymous'} />
</Link>
<div className="lg:ml-3 flex items-center justify-start lg:mb-0 mb-4 pointer-cursor m-l-auto rounded-full border-2 border-white hover:border-gray-200 p-0.5">
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Could we make the border gray-500 when the profile menu is active (dropdown is visible)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems rather involving to do tbh

Copy link
Contributor

@gtsiolis gtsiolis Mar 19, 2021

Choose a reason for hiding this comment

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

True, let's drop this idea for now.

@@ -50,12 +51,16 @@ function ContextMenu(props: ContextMenuProps) {
{props.children}
</div>
{expanded?
<div className={`z-50 ${props.width || 'w-40'} bg-white absolute py-2 right-0 flex flex-col border border-gray-200 rounded-lg space-y-2`}>
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: In the end, this dropdown should use the same spacing as the workspace more actions dropdown as described in #3496 (review).

Let's change the width of the dropdown of the more actions to w-48. This will also use the same width as the side menu. Also, let's change the dropdown spacing from space-y-2 to space-y-1 and py-2 to py-1.

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 went away from the inbetween spacing, because separators would be hard to align in the middle. They are no based on borders.

<div className="lg:ml-3 flex items-center justify-start lg:mb-0 mb-4 pointer-cursor m-l-auto rounded-full border-2 border-white hover:border-gray-200 p-0.5">
<ContextMenu menuEntries={[
{
title: (user && User.getPrimaryEmail(user)) || '',
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: This should be truncated, right?

@svenefftinge svenefftinge force-pushed the se/dashboard-avatar-menu branch from 3f6a5dd to b5dea05 Compare March 19, 2021 11:11
@svenefftinge svenefftinge merged commit 115ce8d into dashboard-v2 Mar 19, 2021
@svenefftinge svenefftinge deleted the se/dashboard-avatar-menu branch March 19, 2021 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants