-
-
Notifications
You must be signed in to change notification settings - Fork 187
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(organizations): account sidenav update for MMOs TASK-977 #5234
Conversation
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.
Noob question: Don't you also need to push the files generated by the script? Or didn't you need to run the generator for this one?
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.
@pauloamorimbr Nope, not necessary since the script gets run on install.
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.
Some small things but everything else looks good
<div className={styles.navGroup}> | ||
<div className={styles.subhead}>{mmoLabel.toUpperCase()}</div> | ||
<AccountNavLink | ||
iconName='users' |
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.
I think this should be 'user'
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.
I'm confused. It's a new icon I'm adding. We have a 'user' icon, but that's for the single-user symbol, not the multiple-user symbol. Let me know if you don't like the name, though. I would have gone with 'members' but users seemed more generic.
name={t('Members')} | ||
to={ACCOUNT_ROUTES.ORGANIZATION_MEMBERS} | ||
/> | ||
{hasAdminPrivileges && ( |
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.
I think we need to have a subscription check here, unless I'm misunderstanding the figma:
Looks like usage should be missing if stripe is not enabled. I'm guessing admins also should not be able to see the usage page if stripe is not enabled (if the owner can't). This is what I currently see:
where dd
is the owner
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.
You're right, good attention to detail, but I am overriding the designs here :) We don't currently hide usage for non-Stripe instances and I didn't see anywhere in the project requirements to indicate hiding it was intended as part of the scope for this project, so I think this is just an oversight in the designs.
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.
As for the part about subscriptions, the language in the designs is a bit off. Where it says "with subscription" and "without subscription", you should read "with Stripe" and "without Stripe."
) { | ||
const showBillingRoutes = | ||
userRole === OrganizationUserRole.owner && isStripeEnabled; | ||
const hasAdminPrivileges = [ |
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.
Feels like this kind of logic will be used a lot. Should we make a organizations utils file for this and the above checks?
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.
You're right, it does seem like this kind of thing will probably end up getting repeated. But I think in this case it will be best to wait and see how it gets repeated and under what circumstances. Then we will know what a util should look like and if it's really going to save us on repetition.
### π£ Summary Handle MMO and non-MMO orgs, as well as org roles, in the account sidebar. ### π Preview steps Based on figma designs [here](https://www.figma.com/design/VIn6Fv2oEpqDHaGoqs6Zt9/%E2%9A%99%EF%B8%8F-KPI-%2F-Organizations-%26-Teams-Feature?node-id=1732-41685&t=XhwXHn8qEGw7X4Oi-4) Requires MMO feature flag 1. Create a user and check to ensure account sidenav remains unchanged on Stripe-enabled and disabled environments. The should be no changes for users who are not members of an MMO. 2. Make user's org an MMO, with them as an owner, and check the sidenav with Stripe enabled and disabled against the Figma designs for org owners. 3. Create another user and add them to the MMO (make sure to delete their personal org). Make them an admin and check against the figma designs as above. 4. Make second user a member (uncheck "admin" on their org user object in django admin) and check against the figma designs as above. 5. Toggle the use_team_label option in constance and see organization/team label change accordingly in sidenav. ---------
ποΈ Checklist
<type>(<scope>)<!>: <title> TASK-1234
frontend
orbackend
unless it's globalπ£ Summary
Handle MMO and non-MMO orgs, as well as org roles, in the account sidebar.
π Preview steps
Based on figma designs here
Requires MMO feature flag