- Notifications
You must be signed in to change notification settings - Fork928
feat(site): add support for presets to the create workspace page#16567
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
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.
Just left a few comments.
Uh oh!
There was an error while loading.Please reload this page.
| value: preset.ID, | ||
| })), | ||
| ]; | ||
| }, [presets]); |
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.
We only useuseMemo when really necessary. In this case, it would be ok to compute this on every render.
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.
Cool. I will remove the useMemo. For my own learning: Can you help me understand why not to use useMemo in this case?
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’s just a matter of adding complexity—caching—in a place where it’s not really needed.
| presets, | ||
| parameters, | ||
| form.setFieldValue, | ||
| ]); |
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'm assuming this effect is to set in the form the default values right? If yes, I think the best place to set them is in the form initialValues prop here
| initialValues:{ |
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.
This effect is not only for default values, but also for if the user chooses a new preset. If they choose a new preset then this effect will autofill the form for them using the new preset values.
| @@ -189,6 +245,31 @@ export const CreateWorkspacePageView: FC<CreateWorkspacePageViewProps> = ({ | |||
| </Alert> | |||
| )} | |||
| {presets.length > 0 && ( | |||
| <FormSection | |||
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.
A form section should be used to group similar fields. I'm usually don't like to have sections with a single field 🤔 but I will leave this up to you. I was wondering if this could not be in the General section.
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.
Understood. There is another issue open to consider the UX and design of this entire page. I have made this change as requested, but I would like to request that we defer further design discussions for the mockups.
Removed myself as a reviewer; this is not my area of expertise. All I'd be able to contribute is vibes or a stamp, neither of which are valuable. |
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.
@SasSwart Just check the Chromatic checks pls 🙏
a17cf03 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This pull request adds support for presets to the create workspace page. This behaviour can be seen in the storybook. This will not be visible in dogfood until we merge support for presets in the provisioners.
There is more frontend work to be done before this is ready for a general release, but this should be sufficient for dogfood testing