-
Notifications
You must be signed in to change notification settings - Fork 1.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
[dashboard] Implement Teams UI (selector, creation wizard, members page, project page) #4401
Conversation
49100ab
to
facd6ca
Compare
@svenefftinge Sure, many thanks! |
8af3ab1
to
f83bfad
Compare
Well, at least the new check works! 🤣
|
0f54090
to
9b8a149
Compare
Alright, this is now ready for review. 🎉 Please take a look! 🙏 In particular:
Notes: I'm happy to make any fixes/adjustments, but in the interest of time, I'll probably make larger changes in follow-up Pull Requests (in order to merge soon and unblock others from working on these pages too.) -- For example, there is still no team name deny-list, so you can create teams called "settings" or "workspaces" 😬 (will fix that next). |
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.
code looks good!
const memberships = await this.teamDB.findMembershipsByTeam(team.id); | ||
await this.guardAccess({ kind: "team", subject: team, memberships }, "get"); | ||
return Promise.all(memberships.map(async (m: DBTeamMembership): Promise<TeamMemberInfo> => { | ||
const member = await this.userDB.findUserById(m.userId); |
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.
Depending on the team size this can become expensive. A single SQL query would be better.
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.
would be good to move this to an issue, so we don't forget :-) |
/werft run 👍 started the job as gitpod-build-jx-teams-ui.40 |
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.
Clicking those tabs feels so good! Thanks for adding this @jankeromnes! 🌟
Left some first-round comments, feel free to open follow up issues for anything that can go in a later PR or ingore any comments that seem minor. 🏀
} | ||
]}> | ||
<div className="flex p-1.5 pl-3 rounded-lg hover:bg-gray-200"> | ||
<span className="text-base text-gray-600 font-semibold">{team?.name || userFullName}</span> |
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.
issue: Reminder to restore the colors for dark theme! These can use the colors of the menu items on the right. Font size can be text-sm
! 🌙
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.
@gtsiolis Unless I'm looking at the wrong spec, the Figma file currently has text-base
(i.e. 16px
) for team member names, not text-sm
(i.e. 14px
).
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.
@jankeromnes You are absolutely right! The initial specs are using 16px
for the main and secondary navigation on the top right which is now still using a 14px
size. May I suggest the following two items:
- Either use the smaller size of align all navigation elements and make sure they are using a
32px
height? - If we go with the specs (
16px
), semibold here is rendering quite bold so I'd either change the font weight to medium or even better enable font-smoothing for the product as in https://github.com/gitpod-io/website/pull/222.
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.
Now tracked in #4471
...(teams || []).map(t => ({ | ||
title: t.name, | ||
customContent: <div className="w-full text-gray-400 flex flex-col"> | ||
<span className="text-gray-800 text-base font-semibold">{t.name}</span> |
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.
issue: Reminder to add colors for dark theme and use text-sm
font size!
<span className="text-gray-800 text-base font-semibold">{t.name}</span> | |
<span className="text-gray-800 text-sm dark:text-gray-300 font-semibold">{t.name}</span> |
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.
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.
Now tracked in #4471
</Link> | ||
<div className="ml-2 text-base"> | ||
{showTeamsUI | ||
? <ContextMenu classes="w-64 left-0" menuEntries={[ |
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: Could we bg-gray-100
on hover and also use this background for the currently active team? 💭
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.
Now tracked in #4471
<Tab name='Recent' setSelection={setSelection} selection={selection} /> | ||
<Tab name='Examples' setSelection={setSelection} selection={selection} /> | ||
<TabMenuItem name='Recent' selected={selection === 'Recent'} onClick={() => setSelection('Recent')} /> | ||
<TabMenuItem name='Examples' selected={selection === 'Examples'} onClick={() => setSelection('Examples')} /> |
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: Could we make this tab menu disappear when creating a new team similar with the specs? Low priority! 🔅
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.
Now tracked in #4471
32ec662
to
fd9006d
Compare
Thanks again for the fantastic reviews! ❤️ Also, phew!, addressed all nits (either here or by opening follow-up issues), and double/triple-checked everything again. One more smoke test on this deployment (because I'm getting tired and had to do some pretty crazy interactive reverts/rebases) and we should be good to merge. 🚀 EDIT: All perfect! 😌 |
Fixes #4355
Fixes #4354
TODO:
https://gitpod.io/<teamname>