Nit: one thing we've been doing recently is bringing in the FC type to give better feedback about accidentally returning wrong values (especially since React 18 fixed the previous issues with the type)

yeah, I think we should still use const and FC and arrow functions. it's a small bit of work to clean up shadcn things as we add them, but there are reasons we've consolidated on writing components the way that we do, and we should definitely stick with it.

I agree, seems like a good idea. This means I should probably create a doc to list all the extra steps needed when importing a shadcn component.

@aslilac it would be useful to know the various reasons that components are currently written as they are.

I'm not sure what we've been doing in core lately, so just making sure I understand: are we going to be exporting styles like this going forward? At first blush, it's not super clear to me if this will serve a benefit

this is definitely another shadcn-ism we should avoid. just put export on the declaration if we need to. (I'm also not sure what value exporting a constant like this provides.)

My original thought was to keep the shadcn components as close as possible to how they are imported but it makes sense to change this since the current codebase convention is not to group all exports at the bottom of the file. I think this pattern is generally used so its easy to look up all exports in one place.

@Parkreiner Since I have seen the export at the bottom pattern more often in other codebases, I am curious if you know why the current pattern exists?

To be honest, I haven't been in enough "big" codebases to notice any "industry-leading" patterns, but Coder was just like this when I started working for them. Though I am a bigger fan of just exporting any relevant values directly

I know that .d.ts files often have all their exports at the bottom, but I don't know if that's a meaningful reference, since those are auto-generated anyway

On a similar note to the one about the style exports, are we changing how we're writing out code now? I few things immediately jump out to me:

• Using a wildcard import for React (which shouldn't matter, since React isn't easy to tree-shake anyway)
• Consolidated exports at the end of the file
• Random library-specific HTML attribute (cmdk-input-wrapper)
• Extensive use of Tailwind's arbitrary variants feature to inject styles for the Cmdk component

Everything in src/components/ui comes directly from shadcn except for multiple-selector. I don't want to assume any existing patterns in the codebase are what we should definitely use going forward. if there is disagreement with how things are done with shadcn, lets bring them up all so I can make a list of everything that needs to be changed when importing a shadcn component.

I feel like I'm missing something, but how are we formatting the Tailwind class names?

uhh, this is a particularly nasty string of class names. it wraps across 4 lines on my enormous screen and immediately triggers my "this is incomprehensible and I should run away" reflex. how could we refactor this to be clearer? maybe for things with lots of complicated selectors like this we should stick with emotion actually?

All this is really doing is styling the underlying cmdk component using the tailwind arbitrary variants feature. The purpose of doing this is because of the assumption that shadcn components are battle tested and are saving us alot of time compared to writing this from scratch. I don't think it would matter much if emotion was used here as the complexity exists because of consuming the cmdk component.

Yeah, to be clear – I think this is an area where the Tailwind abstractions break down – definitely not the fault of anyone here. I understand why the code is the way it is:

• The CMDK component expects styles to be provided via a specific set of exposed class names
• We also can't dynamically process the class names in a function or something, because then Tailwind can't create CSS classes at compile time
• TW isn't great at that, so we have to bust out their arbitrary variants, and make sure that we're satisfying the TW compiler's needs, and the component's styling needs. It's just that the syntax is awkward and ugly

I'm not a fan of how it looks, but if we can shove all the nastiness into a corner and put a nice bow on it, I think it's fine enough

Getting back to my original question, though: what tools do we have to sort the class names automatically? The syntax is bad enough, but if we can't even order the styles by the order that they're applying in, this will be a nightmare to debug

Doing the class sorting is a solved problem when using prettier. Its a work in progress with biome as discussed here. biomejs/biome#1274

This doesn't the logic, obviously, but the fact that it's here has me worried our formatting logic isn't working properly: there's a double-space right after the bg-black/80

Right now biome doesn't help with this. Once this PR is merged I was planning on looking into the issue with Tailwind class sorting using Biome, biomejs/biome#1274

This shouldn't be necessary. The HTML spec makes sure that an onClick handler is also automatically wired up to the onKeyDown event for Enter and the onKeyUp event for Space

The whole thing can be removed

When I test this by removing the onKeyDown handler by tabbing to an item in the multi-select and pressing enter, I am no longer able to unselect/remove the item from the multi-select component.

Okay, in that case, they might be doing some things that break from the spec. I'm okay with keeping it – I would just like a comment explaining that it is necessary

@Parkreiner Can you refer to where in the HTML spec you found this?

btw if I remove onKeyDown, the key that actually works in this case is space instead of enter

Could you explain this comment a bit more? Is this a library need? And just making sure: is it okay that the element used is never rendered on screen?

@Parkreiner I am only seeing line numbers for this comment and the line numbers have changed since the original comment. Which comment is this referring to?

yeah, I think we should still use const and FC and arrow functions. it's a small bit of work to clean up shadcn things as we add them, but there are reasons we've consolidated on writing components the way that we do, and we should definitely stick with it.

this is definitely another shadcn-ism we should avoid. just put export on the declaration if we need to. (I'm also not sure what value exporting a constant like this provides.)

uhh, this is a particularly nasty string of class names. it wraps across 4 lines on my enormous screen and immediately triggers my "this is incomprehensible and I should run away" reflex. how could we refactor this to be clearer? maybe for things with lots of complicated selectors like this we should stick with emotion actually?

I was already against primary, secondary and tertiary as being three bad names, but now we're adding quaternary? this is such a horribly meaningless name

At this point, the best place to discuss the naming issues would be in Figma as that is the source of truth. https://www.figma.com/design/WfqIgsTFXN2BscBSSyXWF8/Coder-kit?node-id=6-2115&m=dev

wait, so it's an error to specify the UUID? why?

this is the same behavior as the idp sync groups and roles feature that went out with orgs. The idea is that if the uuid does not map to an organization, then the organization has been deleted and this is a problem that should be corrected.

does this bring in all of lucide? how does this affect our bundle size?

Only the icons that are used, https://lucide.dev/guide/#code-optimization

The more I think about this, the more I think we should be giving the user a better experience while the page is in a loading state.

Right now, while the entire page is loading, the user only sees a loading spinner, and because updating the title is tied to Helmet, the HTML title stays the same from the previous page, too.

Ideally, I think I'd prefer for the page to be updated so that:

1. The HTML title, page title, and description are updated immediately, since none of that requires fetching. All the data is available synchronously
2. Move the loader inside the main content area, and also update the button so that it's more granularly tied to the loading state, too

To be honest, I feel like we're also kind of blurring what should be a page component concern, and what should be a view component concern

All the header content here is part of the UI view, but because it's not part of the view component, any errors or rendering problems will never show up in Storybook

Not sure if you have any thoughts @aslilac

Just making sure: this is the only description in the Deployment navigation section that has a medium font weight. Is that something we're moving towards with the design tokens?

in the designs, most fonts are 500 but on this page it only needs to be specified here

The reason why I was asking was because all the other pages use font weight 400 (Roman weight). I think it's outside the scope of the PR, but we'll need to make another PR to update those, too

yes there is an issue already created to handle global style changes

Nit: one thing I usually do when I import icons is that I rename them to SomethingIcon, where Something is the role I'm intending for the icon to fill. I find this really helpful when I'm using an icon outside its intended purpose, but even here, I think <CloseIcon /> is more readable than <X />

I think there are pros and cons either way. I like to easily tell what the icon visually looks like and the original name works well for that. Using icon in the name could also be a good way to look up all icons. Overall, I am not a fan of naming imported components for their specific purpose unless its really confusing otherwise.