-
Notifications
You must be signed in to change notification settings - Fork 336
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
feat: public teams #9057
feat: public teams #9057
Conversation
Left a couple comments on the Loom! I'd like to run this through design maintainer review as well since there's a navigational change. UI is fine, just the UX questions asked in the Loom to sort out |
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.
Not tested yet, have few questions:
organizationsRef | ||
) | ||
const hasPublicTeamsFlag = | ||
organizations?.[0]?.viewerOrganizationUser?.user?.featureFlags?.publicTeams |
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.
We check feature flag on backend when we return allTeams
do we need this check on frontend too?
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.
We do because if we query allTeams
and the viewer is a billing leader, all teams in the org will be returned even though they don't have the feature flag. So, we want to check this in the client, and return viewerTeams
if they don't have the flag.
I've added a comment in the code to clarify this.
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.
In this case the question is what is the reason to explicitly allow billing leader to see all teams in API while we not allowing to see them on frontend anyway? Should we remove this condition?
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.
In the teams sections of the org settings, billing leaders can see all teams within the org: https://action.parabol.co/me/organizations/Skc7syWwNox/teams
This was a feature requested by the sales team a few months ago.
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.
Thanks - fixed! |
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.
When I request to join a team and the team lead accepts the request, I would like if I would join directly and wouldn't need to accept the invite.
if (isBillingLeader || isSuperUser(authToken) || hasPublicTeamsFlag) { | ||
const teamIds = allTeamsOnOrg.map((team) => team.id) | ||
const isViewerInOrg = teamIds.some((teamId) => user.tms.includes(teamId)) | ||
if (!isViewerInOrg && !isSuperUser(authToken)) return [] |
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.
+1 Why is this check necessary now? If I'm not on the org, I shouldn't be able to load this Organization
in the first place.
@@ -29,7 +29,7 @@ const EditableTeamName = (props: Props) => { | |||
teamId: id | |||
teamName: name | |||
organization { | |||
teams { | |||
viewerTeams { |
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.
-1 This is used to check for duplicate names in l59, so it should be allTeams
here.
@@ -117,7 +117,7 @@ const NewTeamForm = (props: Props) => { | |||
id | |||
lockedAt | |||
name | |||
teams { | |||
viewerTeams { |
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.
-1 these teams are used for checking for duplicate names in l159
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 are a couple minor things
Agreed! I'll fix that in another issue: #9108 |
@nickoferrall this went through design review. Designers requested we change the icon to be the same for all and just use the difference in shade to differentiate between the tow. They also requested that the "invite request" screen would stay for [x] days before reverting to the "request to join" version, to avoid spamming with requests. I suggested this improvement as a fast follow so it doesn't block this PR. |
Sounds good @acressall. I've updated the icon and created an issue for the request screen: #9117 |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This PR implements a first version of public teams.
Users with the feature flag:
See demo: https://www.loom.com/share/00cae6305c93462d90d32b18601688fd
This PR does not include private teams. If any users are unhappy with public teams, we can remove the feature flag. The modal doesn't include the name of the team the viewer wants to join, but this can be added in a later PR.
A concern I have is that orgs with many teams, like Parabol, may find that the sidebar becomes too crowded with all of the public teams. I think this is better than having a scroll bar within a scroll bar - see Loom example here.
To test
publicTeams
org feature flag