-
Notifications
You must be signed in to change notification settings - Fork 15
Limited collaborator UI #2960
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
base: main
Are you sure you want to change the base?
Limited collaborator UI #2960
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
the div wrapping the radios was messing up the way RadioFieldDyn passes checked state to its child radios because they're expected to be direct children
6e0af82 to
4b3731d
Compare
Okay; just for clarity, does the "Add user" modal remain a side modal, because you have to enter the name and select the role, or is it conceptually a single input and should be a regular modal? |
| name="roleName" | ||
| label={`${scope} role`} | ||
| required | ||
| control={control as Control<AddUserValues>} |
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.
Why this doesn't work without the cast: RadioFieldProps has this too-clever parseValue thing and then RadioFieldDynProps omits parseValue. I think this may be a TS bug. Here's a repro of the issue stripped down to two lines: https://tsplay.dev/ND13Om
I have two workarounds, one of which is to turn parseValue into a regular optional, the other is to inline theRadioFieldDyn call in the RoleRadioField call sites. I think I prefer the latter. I'll figure it out.
console/app/components/form/fields/RadioField.tsx
Lines 42 to 51 in 6764ba5
| } & (PathValue<TFieldValues, TName> extends string // this is wild lmao | |
| ? { parseValue?: never } | |
| : { | |
| /** | |
| * Radio field values are always strings internally, but sometimes we | |
| * want them to represent something else, like a number. `parseValue` is | |
| * required if and only if the value type does not extend `string`. | |
| */ | |
| parseValue: (str: string) => PathValue<TFieldValues, TName> | |
| }) |
console/app/components/form/fields/RadioField.tsx
Lines 102 to 107 in 6764ba5
| export type RadioFieldDynProps< | |
| TFieldValues extends FieldValues, | |
| TName extends FieldPath<TFieldValues>, | |
| > = Omit<RadioFieldProps<TFieldValues, TName>, 'parseValue' | 'items'> & { | |
| children: RadioElt | RadioElt[] | |
| } |
|
The spacing bug seems to be that the checkbox component needs |



Once oxidecomputer/omicron#9299 is merged to Omicron, we'll want to use the new permissions in the web console as well. This PR bumps Omicron (will want to bump again before merging this) and also redesigns the access forms to use radio buttons, which allow us to show all options on the screen, with descriptions of what each role enables.