feat: add premium license banner for custom roles page #14595
Conversation
8d21b76 to
a8f6d9c
Compare
| <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.
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.
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.
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.
I really 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.
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.
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.
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.
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
There was a problem hiding this comment.
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.
ooooh, purple divider looks rad
resolves #14318