-
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: [3/n]: Role Based Front End Changes #27327
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.
Looks great, just a few comments.
Could you also confirm the role at org is being saved new users?
frontend/src/scenes/userLogic.ts
Outdated
isUserNonTechnical: [ | ||
(s) => [s.user], | ||
(user) => { | ||
const technicalRoles = ['engineering', 'founder'] |
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'd argue data
and product
are technical so it's just sales
, marketing
, leadership
and other
that are not.
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've used the ICP scoring system where we give leadership and engineering the highest score. Product also gets a high score if they have a github account, but we won't know that here.
I'll revisit this when I dig deeper into ICP definitions
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.
Given we want to keep the experience similar for most users and roll this out to the most non technical users I'd argue we don't need to follow the ICP score exactly. We can also expand it over time but it may be better to limit the exposure of this feature to start.
Size Change: +245 B (+0.02%) Total Size: 1.11 MB ℹ️ View Unchanged
|
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.
One small nit but not a blocker.
@@ -15,6 +15,16 @@ import { OnboardingStep } from '../OnboardingStep' | |||
import { sdksLogic } from './sdksLogic' | |||
import { SDKSnippet } from './SDKSnippet' | |||
|
|||
export function InviteHelpCard({ className = '' }: { className?: string }): JSX.Element { |
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.
Nit: why pass in class name if it's not used?
… into onbarding_frontend
… into onbarding_frontend
… into onbarding_frontend
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
Problem
The invite users option gets hidden at the bottom of the screen. Since this option will likely be used by non-technical users. I'm making changes to show this to users higher up on the screen when the user.
I've also tagged along ordering the SDK list to show HTML snippet on top of the SDK list for non technical users since they are more likely to use HTML than Javascript.
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
Cloud: Yes
How did you test this code?
Verified if the changes show up for non-technical users
Verified technical users don't see any changes
Verified changes are applied only to product analytics