-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: update organization member abilities #27232
Changes from 3 commits
c926bde
0f27038
e262d6e
f77d28b
b56814e
7b468b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the reason for making it so that members cannot invite new members generally? Since we don't charge for seats I don't think this is a good idea. Getting more people into posthog is always better, and bottlenecking that through admins and owners is probably a bad thing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I've went back and forth on this. Generally it's not great practice to allow anyone to join your organization but I agree with you. I think I'll wind this back to how it is today and on another PR add a configurable option for organizations to set this - could be a teams add-on feature. We've had multiple customers ask for limiting who can invite. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated - there are no changes on this PR for sending invites, although it still blocks deleting invites. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,9 +11,16 @@ export interface SSOSelectInterface { | |
loading: boolean | ||
onChange: (value: SSOProvider | '') => void | ||
samlAvailable: boolean | ||
disabledReason?: string | null | ||
} | ||
|
||
export function SSOSelect({ value, loading, onChange, samlAvailable }: SSOSelectInterface): JSX.Element | null { | ||
export function SSOSelect({ | ||
value, | ||
loading, | ||
onChange, | ||
samlAvailable, | ||
disabledReason, | ||
}: SSOSelectInterface): JSX.Element | null { | ||
const { preflight } = useValues(preflightLogic) | ||
|
||
if (!preflight) { | ||
|
@@ -46,7 +53,7 @@ export function SSOSelect({ value, loading, onChange, samlAvailable }: SSOSelect | |
value={value} | ||
options={options} | ||
loading={loading} | ||
disabledReason={loading ? 'Cannot change while loading' : undefined} | ||
disabledReason={loading ? 'Cannot change while loading' : disabledReason} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this component not handle disabling while loading? I kinda figure it would since it has the loading prop. if not, it probably should? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does, but this is just adding a tooltip over it while it's loading. The loading props just disables it, doesn't give a tooltip. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, might be worth changing, so if no reason it just says "loading..." Otherwise can override if you want a custom loading message with disabledReason There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm hestitant to make changes that affect such a high number of parts of the app - that would make it so anytime a button is loading there is a tooltip over it showing text (the LemonSelect uses LemonButton). I think it's better to have a more explicit option like this for when you actually want a tooltip. |
||
fullWidth | ||
onChange={onChange} | ||
/> | ||
|
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.
Our pattern is usually
disabledReason
to force someone to give a reason why something is disabled, which is a better user experience. Should we follow that pattern?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.
Disabled reason looks to be used for buttons and such but just disabled on inputs because we don't show the tooltip on those.
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.
Yeah, just wondering if we should change that