- Notifications
You must be signed in to change notification settings - Fork1k
feat: implement Premium features page using shadcn/ui and Tailwind#15094
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
a8f0643 to45daf0aComparec020034 to18da110Compare18da110 tof08bee2Comparewhat's the difference between the two screen shots? I recognize the first design from figma, but I don't understand the second screenshot |
site/components.json Outdated
| "components":"@/components", | ||
| "utils":"@/lib/utils", | ||
| "ui":"@/components/ui", | ||
| "lib":"@/lib", | ||
| "hooks":"@/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.
I have a feeling these are all very wrong
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 the default generation by shadcn when runningpnpm dlx shadcn@latest init. components.json is only used when using the shadcn cli. I haven't really decided what to do about this yet.
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.
For example, one question is if we should keep shadcn components in a separate folder currently itsui to make it clear which ones they are? or just place them in components with everything else.
site/package.json Outdated
| "@mui/system":"5.16.7", | ||
| "@mui/utils":"5.16.6", | ||
| "@mui/x-tree-view":"7.18.0", | ||
| "@radix-ui/react-icons":"1.3.0", |
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.
we're using radix iconsand lucide?
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 both were added when doing the shadcn installation, lucide is used by the default style and radix icons is used by the new-york style. Since we are going to use neither style, I need to update this for the icon library we are using going forward.
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.
Since it looks like Christin is using Lucide icons in Figma, I am going to update this to use those.
| module.exports={ | ||
| plugins:{ | ||
| tailwindcss:{}, | ||
| autoprefixer:{}, | ||
| }, | ||
| } |
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 thought tailwind was moving away frompostcss. am I misremembering?
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.
postcss seems to required when using vite
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, the have their own custom parser, so they're slowly moving away from PostCSS. But they're still supporting it for now
site/vite.config.mts Outdated
| testHelpers:path.resolve(__dirname,"./src/testHelpers"), | ||
| theme:path.resolve(__dirname,"./src/theme"), | ||
| utils:path.resolve(__dirname,"./src/utils"), | ||
| "@":path.resolve(__dirname,"./src"), |
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.
we already have aliases for all of the top level folders, let's not add multiple ways to do things pls
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 a left over piece from the shadcn installation
| "include": ["**/*.ts","**/*.tsx"], | ||
| "exclude": ["node_modules/","_jest"], | ||
| "types": ["@emotion/react","@testing-library/jest-dom","jest","node"] | ||
| "compilerOptions": { |
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.
idk how this wasn't tabs already, but thank you!
| Template Permissions | ||
| </span> | ||
| <br/> | ||
| <spanclassName=" font-medium"> |
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.
| <spanclassName="font-medium"> | |
| <spanclassName="font-medium"> |
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.
Once we get Tailwind class sorting figured out, this shouldn't happen anymore.
| exportconstPremiumPageView:FC<PremiumPageViewProps>=({ isEnterprise})=>{ | ||
| returnisEnterprise ?<EnterpriseVersion/> :<OSSVersion/>; | ||
| }; |
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.
oh weird. I don't remember seeing this in the designs. did sales ask for this 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.
Yes, It was requested and the designs have been updated since then.
site/src/contexts/ThemeProvider.tsx Outdated
| // value could be anything, like an empty string. | ||
| useEffect(()=>{ | ||
| constroot=window.document.documentElement; |
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.
| constroot=window.document.documentElement; | |
| constroot=document.documentElement; |
site/src/components/ui/button.tsx Outdated
| import{typeVariantProps,cva}from"class-variance-authority"; | ||
| import*asReactfrom"react"; | ||
| import{cn}from"@/lib/utils"; |
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 we're gonna wanna change this path to fit in with what we've already got. maybe modules/classes.ts or utils/classes.ts? I'd lean towards modules as I don't really like the name "utils", everythingis a util but nothing shouldjust be a util, y'know?
site/src/App.tsx Outdated
| {showDevtools&&<ReactQueryDevtoolsinitialIsOpen={showDevtools}/>} | ||
| </QueryClientProvider> | ||
| </HelmetProvider> | ||
| <CacheProvidervalue={cache}> |
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 this provider actually go insideThemeProvider instead, since it's already doing emotion related stuff?
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.
yes, seems like that is fine.
The first is for Enterprise users, the second for OSS users. |
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.
Wasn't sure if you were looking for feedback, or want to treat this as a "100% real" PR, and need approval. Approving just in case, because I don't see anything egregious
I left a comment about one pattern that I'm not a huge fan of, but in practice, it might not be a huge deal. Mainly worried about dev experience, and making sure the right props are getting applied to the right elements
| module.exports={ | ||
| plugins:{ | ||
| tailwindcss:{}, | ||
| autoprefixer:{}, | ||
| }, | ||
| } |
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, the have their own custom parser, so they're slowly moving away from PostCSS. But they're still supporting it for now
| ); | ||
| }, | ||
| ); | ||
| Button.displayName="Button"; |
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.
Seems like a good idea, although if its required we will have to add it to all existing components. shadcn components always have thedisplayName
| useEffect(()=>{ | ||
| constroot=document.documentElement; | ||
| root.classList.remove("light","dark"); | ||
| if(themePreference==="auto"){ | ||
| root.classList.add(preferredColorScheme); | ||
| }else{ | ||
| root.classList.add(themePreference); | ||
| } | ||
| },[themePreference,preferredColorScheme]); |
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'm not as familiar with the logic for the theme. Is there a reason we're not attaching the remove functionality to a cleanup function?
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.
Thats a good idea to add the cleanup function. I originally only wanted to add or remove thedark class as thats all that needed and not touch the light class but I think this code is a bit simpler consideringpreferredColorScheme
| constcache=createCache({ | ||
| key:"css", | ||
| prepend:true, | ||
| }); |
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 add a comment on why we're defining a custom cache override? I'm guessing this is to make Emotion and Tailwind play nicely with each other, but I'm not 100% sure
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 to allow using Tailwind on MUI components as described here:https://mui.com/material-ui/integrations/interoperability/#tailwind-css
This is actually not needed if never use Tailwind on MUI components and only use Tailwind with new components. I was adding it here just in case it is useful but actually maybe its better we don't add this. Any opinions either way?
| --surface-error:#450a0a; | ||
| --border-default:#e4e4e7; | ||
| --border-error:#ef4444; | ||
| --radius:0.5rem; |
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 sure I'm understanding this correctly: does this bind the border radius value to the root em for the browser?
If so, do we want to do that? If the user ever increases or decreases their default font size, that would cause the borders to increase/decrease roundness and potentially look wonky, right?
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.
--radius is used in the tailwind config here,
borderRadius: { lg: "var(--radius)", md: "calc(var(--radius) - 2px)", sm: "calc(var(--radius) - 4px)",},by default Tailwind uses border-radius: 0.5rem; for rounded-lg so nothing is changing there. Changing the zoom level in the browser should by fine as well.
These were both added by the shadcn installation. I was keeping this for now until we know exactly why shadcn is adding this override to Tailwind for md and sm.
| constEnterpriseVersion:FC=()=>{ | ||
| return( | ||
| <divclassName="max-w-4xl"> |
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.
Should this bemax-w-prose?
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 max width is used to set the position of the button. usingmax-w-prose doesn't push the button far enough to the right.
| <ButtonasChild> | ||
| <ahref="https://coder.com/contact/sales"className="no-underline"> | ||
| Upgrade | ||
| </a> | ||
| </Button> |
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 code might be perfectly fine, but seeing this immediately makes me a little worried. Part of the reason why I haven't liked MUI is because they've tried really hard to blur the lines of links and buttons, and the resulting props are kind of a mess, and the actual function component is weirdly megamorphic
I'd need to look into exactly how Radix'sSlot component works under the hood, but I'm okay with this if the developer experience is still good, and there's no risk of "mixing metaphors" by putting button-exclusive props onto anchors
| href={docs("/admin/users/organizations")} | ||
| > | ||
| <spanclassName="flex items-center"> | ||
| <h2className="text-sm font-semibold m-0"> |
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.
Should there be a need for anm-0? Seeing that makes me think that Tailwind's default reset isn't being applied correctly. Though I'm not sure if that's actually expected while we're migrating off MUI
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.
in the tailwind config the css reset is turned off.
corePlugins: {preflight: false,},So we are going to need to do things like this in the short-term or maybe we can find another workaround
| ); | ||
| }; | ||
| constOSSVersion:FC=()=>{ |
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.
The spacing on this version seems a little off, and doesn't really line up with what's happening in the Premium version
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.
In Figma, there are really 2 separate designs for OSS/Enterprise with different spacings.
| }, | ||
| colors:{ | ||
| content:{ | ||
| primary:"var(--content-primary)", |
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.
Wondering if it might be good to define default values for thevar calls, just in the off chance that a variable isn't defined
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.
Then we have potentially 2 sources of truth for the colors. I would imagine that once this setup, it would rarely change.
Parkreiner commentedNov 2, 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.
Now that I think about it, I wonder if it might be good to see if we can request copies of Refactoring UI for everyone. Tailwind is basically all the ideas in that book turned into a library, so reading through it might help make some of the ways Tailwind does things seem feel less arbitrary |
40ce761 to0e364fcCompare| "editor.defaultFormatter":"biomejs.biome", | ||
| "editor.codeActionsOnSave": { | ||
| "quickfix.biome":"explicit" | ||
| // "source.organizeImports.biome": "explicit" |
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: should we just remove it?
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.
@aslilac Were you keeping this around for some reason?
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 was hoping to re-enable it after Biome pushed a couple more bug fix releases. Enabling this would mean that it'd sort imports on file save (and avoid annoying CI failures) but someone (I think Asher?) encountered a really nasty bug with it. It's been a few months though, so maybe we should enable it!
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.
It looks really nice!
cff53e6 to0614f57Comparegithub-actionsbot commentedNov 4, 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.
🚀 Deploying PR 15094 ... |
230244e to87fd564Compare302ca4f to962045cCompare
chrifro left a comment
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 you adjust the text to use sentence case instead of title case? So only sentence starts and proper names are spelled with a capital letter. This applies to the text and the button. FYI, Matt agreed to the change [1]. I also updated the copy inFigma
Note: unfortunately, I the PR deployment didn't work for me so I was unable to review the UI. Atif is working on fixing that [1] 🙇♀️
d4131ba intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
resolvescoder/internal#176
This is a proof of concept to showcase the use of Tailwind + shadcn/ui. The goal here was to implement a lower complexity page to reduce the initial risk and highlight the specific global changes needed.
Changes: