- Notifications
You must be signed in to change notification settings - Fork928
feat: add premium license banner for custom roles page#14595
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
8d21b76 toa8f6d9cCompareThere 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! I did some QA and it works as expected 👍 good job
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.
Tentatively approving so that you're not blocked, but I think there's a few changes that can be made to make the component logic more reusable and foolproof
| <div> | ||
| <Stack direction="row" alignItems="center" css={{ marginBottom: 24 }}> | ||
| <h5 css={styles.title}>{message}</h5> | ||
| <EnterpriseBadge /> | ||
| {type === "enterprise" ?<EnterpriseBadge /> : <PremiumBadge />} |
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.
My main worry with this setup is that because the conditional "flip" for displaying the text is getting repeated so much, there's a higher risk of someone making a typo and accidentally mixing text and bullet points between paywall types
I know it isn't DRY, but just to keep things firmly grouped together, my preference would be to have two variants of the component (that can be kept un-exported as private implementation details) – one for premium and one for enterprise
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.
Basically, I don't think there's a great way to fully DRY up the code, so we have to choose between DRYing up the content or the markup/styling. But because this potentially involves a lot of money, I'd always choose content and making sure customers can't ever get incorrect information. A single missed!== instead of a=== breaks that
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 was meant to be a temporary solution as I originally thought the enterprise license would only remain for a short time longer. But now I am going to update this to completely remove the enterprise license and leave only the premium
| maxWidth: 920, | ||
| margin: "auto", | ||
| minHeight: 280, | ||
| marginBottom: 32, |
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.
Ireally disagree with having any kind of external margin value on a general-purpose component
Again, maybe not DRY, but since the external spacing is strictly something the parent component would need to be worried about, I'd move that logic here. The margin isn't inherently tied to the content of the component, and now we can't use it for other purposes without having a hard-coded margin value be enforced
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.
got it, removed the marginBottom
site/src/theme/branding.ts Outdated
| /** Colors for enterprise are temporary and will be removed when the enterprise license is removed */ | ||
| enterprise: { | ||
| background: string; | ||
| border: string; | ||
| }; |
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.
Is there any context for what the plan is for our license types going forward? Like, is there a Notion doc page or something?
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.
No, but I believe the plan going forward is to only have the premium license.
| color: theme.palette.text.secondary, | ||
| fontSize: 16, | ||
| maxWidth: 460, | ||
| fontSize: 14, | ||
| }), | ||
| separator: (theme) => ({ |
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.
Making a note for myself: we should update our color theme so that it works better on colored backgrounds. The gray divider color looks really washed out on a saturated colored background
ParkreinerSep 9, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this 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.
Before:
After:
Same thing applies to the border around the button, but I don't want to pop open an MUI library component
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.
ooooh, purple divider looksrad
Uh oh!
There was an error while loading.Please reload this 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.
I think the content updates look good, but I'm going to defer to Kayla's opinion on the color usage
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.
thank you! much happier with this 😄
7de576b intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
resolves#14318