-
Notifications
You must be signed in to change notification settings - Fork 42
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
Create a "Your Teams" page in the User prefs #403
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.
This worked really nicely for me @whimsicallyson . I was able to switch between my tenants and leave one with no problems - really good to have the warning first too. One slight issue that I think is more from Cloud than this PR but it affects it. Then a few small thoughts on design and wording.
isRemovingUser: false, | ||
removeTenant: null | ||
} | ||
}, |
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.
Could we refetch user on mount? (I think the vuex action is getUser?) I accepted an invitation but couldn't see that team until I refreshed my page.
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.
4 teams in sideNav but only 3 in the teams page.
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.
so I've added a getUser
and getTenants
after accepting or declining a team invite, but its not updating the way I'd expect--specifically, I'd like to see the action icons get replaced with the relevant ones after joining a team, but the old ones just go away and are replaced with a blank space :(
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 most likely because of https://github.com/PrefectHQ/ui/pull/403/files#diff-ceb8a586ffaa424f678e44816b6a17d8aa390c65ad1728a6cb1360570c4bbd3eR227, https://github.com/PrefectHQ/ui/pull/403/files#diff-ceb8a586ffaa424f678e44816b6a17d8aa390c65ad1728a6cb1360570c4bbd3eR242, and https://github.com/PrefectHQ/ui/pull/403/files#diff-ceb8a586ffaa424f678e44816b6a17d8aa390c65ad1728a6cb1360570c4bbd3eR257. You're referencing a computed prop by calling its getter method instead of its reactive prop - try currentMemberships.includes(item.id)
instead.
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.
still not sure its working...
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 something like async mounted() { await this.getTenants() },
should work?
https://blog.logrocket.com/introduction-to-vue-lifecycle-hooks/
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.
This works well @whimsicallyson - I left a few comments and agree with @zhen0's review. In addition, I noticed that the table is showing a tenant to which I've been invited but haven't accepted or declined but isn't distinguishing between it and teams I'm already a member of. It'd be good for some visual indicator that it's a pending team (and maybe allow accepting/declining from this table as well? Could re-use the component from the notifications or the tenant switcher in the sidenav).
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.
This is looking really nice @whimsicallyson - the new icons work well and alignment is much better. I've added a suggestion for getTenants() on mount that I think will help with making sure all tenants show up.
Adding getTenants still wouldn't add/show an invitation if the invitation arrives while a user is on the teams page - one way round that would be to add a tenant query instead of using vuex. But I'm not sure it's a major issue so not something I think we need to do here - but maybe @znicholasbrown has an opinion on that?
isRemovingUser: false, | ||
removeTenant: null | ||
} | ||
}, |
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 something like async mounted() { await this.getTenants() },
should work?
https://blog.logrocket.com/introduction-to-vue-lifecycle-hooks/
src/pages/UserSettings/Teams.vue
Outdated
color="primary" | ||
v-on="on" | ||
@click="handleSwitchTenant(item)" | ||
><v-icon>fas fa-eye</v-icon></v-btn |
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.
this eye is so much heavier than the checkmark, I'm tempted to either shrink it or find a different icon. Anyone else feel strongly?
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 font awesome icons tend to be larger for some reason. You could add a 'small' prop to the v-icon?
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.
I like swap_horiz, my feeling is that declining an invite and leaving a team are close enough to the same thing that they can have the same icon.
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.
LGTM @whimsicallyson !
PR Checklist:
CHANGELOG.md
Describe this PR
Implements the enhancement in #194 to make a page with a table of all the teams/tenants you're a part of, with the ability to switch between them (switching will take you to the dashboard) or leave teams you don't need to be in. A few comments inline...