-
Notifications
You must be signed in to change notification settings - Fork 937
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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? |
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 defined gap: 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 for spacing={
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
Outdated
Show resolved
Hide resolved
…PageView.tsx Co-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
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.