-
Notifications
You must be signed in to change notification settings - Fork 889
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2d8b95a
to
0c7f4c1
Compare
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 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
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) using Map.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.
Uh oh!
There was an error while loading. Please reload this page.