Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: implement CRUD UI for IDP organization sync settings #15503
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: implement CRUD UI for IDP organization sync settings #15503
Changes from 1 commit
d0baa8c
6463076
f714986
072d775
f6e286c
acf8cbe
ad1aa84
b01588d
6203d04
bc37998
2c2246d
6971f91
6e87130
a1b6e79
36c8706
a4e66c2
f7be49f
90a65fe
c282765
40155bc
a129820
8edd4b4
e54e5ca
bb16bd1
678a91d
a13d9b2
1b8efd1
137c25c
46a4646
85a455c
a9e06f1
d921b08
ba665b4
8a0e789
234c0fb
7d88a38
c6c9b16
5322a4e
eab8a0d
721d5d4
8986cbd
89a7b12
148dca7
0c338b0
0cadc8b
1d7153b
a914c54
bd307c0
95bfd17
82bd850
6eb6987
3822b2e
fe17b1c
e75e179
c6e07ff
a3b9b27
0601164
b01e5d3
1c5b6ef
6e2ee08
ae30db0
ac74cdc
a99dca5
4c00959
568a5a9
c313439
506223d
f223ab7
94ff4ca
9101e0e
5ca2c0c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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: 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)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, I think we should still use
const
andFC
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.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 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.
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 it would be useful to know the various reasons that components are currently written as they are.
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 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
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 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.)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 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?
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.
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 anywayThere 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.
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:
cmdk-input-wrapper
)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.
Everything in
src/components/ui
comes directly from shadcn except formultiple-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.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 feel like I'm missing something, but how are we formatting the Tailwind class names?
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.
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?
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.
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 thecmdk
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.
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:
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 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
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.
Doing the class sorting is a solved problem when using prettier. Its a work in progress with biome as discussed here. biomejs/biome#1274