-
Notifications
You must be signed in to change notification settings - Fork 2
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
Austenem/CAT-898 R template updates #3540
Conversation
context/app/static/js/components/workspaces/SelectableTemplateGrid/SelectableTemplateGrid.tsx
Outdated
Show resolved
Hide resolved
context/app/static/js/components/workspaces/NewWorkspaceDialog/hooks.ts
Outdated
Show resolved
Hide resolved
context/app/static/js/components/workspaces/SelectableTemplateGrid/SelectableTemplateGrid.tsx
Outdated
Show resolved
Hide resolved
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.
Looks good to me. Just two minor comments.
context/app/static/js/components/workspaces/TemplateGrid/TemplateGrid.tsx
Outdated
Show resolved
Hide resolved
context/app/static/js/components/workspaces/NewWorkspaceDialog/hooks.ts
Outdated
Show resolved
Hide resolved
selectItem={selectItem} | ||
selectedTemplates={selectedTemplates} | ||
disabledTemplates={disabledTemplates} | ||
trackingInfo={{ category: WorkspacesEventCategories.WorkspaceDialog }} | ||
jobType={jobType.value as string} |
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 this assertion necessary?
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.
Without this, jobType.value
could potentially be a string array - however I believe the workspaceJobTypeIdField
defines the field value as a string.
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.
It looks like workspaceJobTypeId
isn't defined in FormType
, which only contains templates: string[]
since it extends FormWithTemplates
:
Since there's no property name that matches workspaceJobTypeIdField
in the form type, TS treats this as either a string or string-array by default.
I believe that extending FormType to include workspace ID should work, e.g.:
interface FormWithWorkspaceJobId extends FormWithTemplates {
workspaceJobTypeId: string;
}
...
interface ControllerProps<FormType extends FormWithWorkspaceJobId> {
...
function SelectableTemplateGrid<FormType extends FormWithWorkspaceJobId>({
I tested this approach out to see if it would work, but it doesn't seem to do the trick, oddly enough.
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.
Should we file a ticket to resolve this later?
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.
Seconding John's feedback, but no further ideas for improvement on my end 👍
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.
@austenem let's file a ticket to figure out the type issues.
Summary
Updates the Launch Workspace and Edit Templates dialogs to account for new blank R template. Includes the following changes:
Design Documentation/Original Tickets
CAT-898 Jira ticket
Figma mockups
Testing
Tested by manually going through both workspace dialogs and checking the listed changes.
Screenshots/Video
Checklist
CHANGELOG-your-feature-name-here.md
is present in the root directory, describing the change(s) in full sentences.