- Notifications
You must be signed in to change notification settings - Fork928
feat: add provisioners view to organization settings#14501
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
Asking this mainly because I want to update the docs and want to fully understand the terminology. How did you decide that Provisioner.tsx and ProvisionerTag.tsx are modules and something like OrganizationAutocomplete or Paywall is a component? Should OrganizationAutocomplete or Paywall actually have been modules? |
aslilac commentedSep 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.
I think |
Nice, I like the distinction that Components could be technically be used in any project and still make sense. |
> | ||
<PageHeaderTitle>Provisioners</PageHeaderTitle> | ||
</PageHeader> | ||
<Stack spacing={4.5}> |
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 the way this is definedgap: spacing * 8,
sort of constrains the UI to consistent spacing in multiples of 8 if we only use whole numbers for spacing. I would argue that for consistency we should keep whole numbers or rethink the spacing * 8 definition.
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 should rethink the* 8
definition. If you do a search forspacing={
you'll see that the codebase is already covered in.5
spacings. 😅 This is just the spacing that the health page already uses, and I think it looks right.
site/src/pages/ManagementSettingsPage/OrganizationProvisionersPageView.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
…PageView.tsxCo-authored-by: Jaayden Halko <jaayden.halko@gmail.com>
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.
LGTM
84922e2
intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closes#14436
Warnings aren't implemented yet on the backend, but the rest of this should be ready to go! I can handle those as part of another PR to avoid blocking this work if they're not done soon.