NotificationsYou must be signed in to change notification settings
Fork924
Star10.1k

fix: improve provisioner details layout and show count line#14749

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

Jump to bottom

Merged

aslilac merged 6 commits intomainfromlilac/add-provisioner-count

Sep 20, 2024

Merged

fix: improve provisioner details layout and show count line#14749

aslilac merged 6 commits intomainfromlilac/add-provisioner-count

Sep 20, 2024

Conversation

Copy link

Member

aslilac commentedSep 20, 2024

Make the provisioner tiles more responsive to page width
Show the number of groups and provisioners at the top of the page
Better tags handling for PSK groups

きれい

1ba2ae1

github-actionsbot assignedaslilac

Sep 20, 2024

aslilac requested a review fromParkreiner

September 20, 2024 18:37

とても大きい

d8b8884

Parkreiner approved these changes

Sep 20, 2024

View reviewed changes

Copy link

Member

Parkreiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good!
(Also, there's a typo in Jaayden's name – it says "Jyeden", as in rhymes with "chai")

site/src/modules/provisioners/ProvisionerGroup.tsx

Comment on lines 33 to 39

		interface ProvisionerGroupProps {
		readonly buildInfo?: BuildInfoResponse;
		readonly keyName?: string;
		readonly keyName: string;
		readonly keyTags: Record<string, string>;
		readonly type: ProvisionerGroupType;
		readonly provisioners: readonly ProvisionerDaemon[];
		}

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Is there a reason why we haven't been using theReadonly utility type more? I can't help but feel like this is a little less typing for just as much type-safety. Plus, I don't think any of our code will ever need to take advantage of interface merging

typeProvisionerGroupProps=Readonly<{buildInfo?:BuildInfoResponse;keyName:string;keyTags:Readonly<Record<string,string>>;type:ProvisionerGroupType;provisioners:readonlyProvisionerDaemon[];}>

I know our entire API types file uses interfaces, but I'm a little fussed about that, since it's all auto-generated

Copy link

MemberAuthor

aslilacSep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

yeah, I've just been trying something 🤷‍♀️ idek if we get any benefit out of it when we immediately destructure the props off of the object in the component