- Notifications
You must be signed in to change notification settings - Fork905
feat: add experimental workspace parameters page for dynamic params#17841
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
2d8b95a to0c7f4c1CompareThere 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 isn't a very general purpose hook. could we put this in modules/workspaces or something instead?
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 hook will eventually be used a many different places, TemplateEmbedPage, WorkspaceSettingsPage, CreateWorkspacePage, UpdateBuildParametersDialog, BuildParametersPopover, WorkspaceParametersForm. Its not general purpose but it also doesn't feel like it should be buried in a subfolder. Maybe we don't currently have a good place for things like this?
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.
Moved to modules/hooks, modules are the place to gather all coder business logic that is not in pages so seems like the best place to put coder specific non-general purpose hooks
Uh oh!
There was an error while loading.Please reload this page.
| newParameterValues.some( | ||
| (p) => | ||
| !currentFormValues.find( | ||
| (formValue) => | ||
| formValue.name === p.name && formValue.value === p.value, | ||
| ), | ||
| ); |
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 is O(n^2), but could pretty easily be rewritten to be O(n) usingMap.groupBy or something similar
I guess it was already like this tho 🤷♀️
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.
Good call out, I can improve.
cb7ce18 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.