-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Role editor: hierarchical validation and tabs #49546
base: bl-nero/slidetabs
Are you sure you want to change the base?
Conversation
}; | ||
|
||
export type KubernetesResourceKind = |
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.
is there some backend equivalent that we are keeping this in line with? Could we maybe add a comment linking to it?
<H2>{isCreating ? 'Create a New Role' : role?.metadata.name}</H2> | ||
<Flex alignItems="center" mb={3} gap={2}> | ||
<Box flex="1"> | ||
<H2>{isCreating ? 'Create a New Role' : role?.metadata.name}</H2> |
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.
Mentioning "Edit Role" is not necessary for editing? Edit Role {role?.metadata.name}
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.
Right, this may be clearer.
isProcessing={isProcessing} | ||
onChange={setStandardModel} | ||
/> | ||
<div id={standardEditorId}> |
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 is <div id={standardEditorId}
needed?
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.
These are used to associate the tab switcher tabs with the switched panels. I'm adding a comment.
}; | ||
export type WindowsDesktopSpecValidationResult = RuleSetValidationResult< | ||
typeof windowsDesktopSpecValidationRules | ||
>; |
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: all the rules functions in this file would have a better home in a separate rule.ts
file
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.
Yes, I agree. The only reason I didn't make it is that I'm constantly in progress with more PRs, so after the version cut, I'm planning to split both the StandardEditor file, as well as this one.
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.
(Oh, wait. This is actually new code. It can be easily extracted.)
This PR moves the role editor UI to a two-level tab hierarchy. It also uses a new approach to validation, which is necessary, because the design requires errors to be visible on multiple levels of the hierarchy. New validation rules are also added to bring the UI up to reasonable parity with the backend requirements.
To turn on the new UI, go to developer tools -> Application -> Local storage and add a
grv_teleport_use_new_role_editor
key set totrue
. This will be lifted once UI is good to be released.Demo
Requires #49545
Contributes to #46612