Skip to content

feat: Adjust forms to include Rich Parameters #5856

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 48 commits into from
Feb 1, 2023
Merged

feat: Adjust forms to include Rich Parameters #5856

merged 48 commits into from
Feb 1, 2023

Conversation

mtojek
Copy link
Member

@mtojek mtojek commented Jan 25, 2023 •
edited
Loading

Related: #5574

Changes:

  • feat: modified Create Workspace form to include rich parameters
  • feat: submit Rich Parameters
  • finish validation rules for Create Workspace
  • feat: support for URL params http://localhost:3000/templates/docker-2/workspace?param.Compute%20instances=7
  • unit tests for Create Workspace (frontend)
  • feat: update workspace build to use new or change parameters (support for mutable)
  • feat: "Build parameters" button in dropdown appears conditionally if the template uses rich parameters.

Screenshot 2023-01-31 at 19 29 39

Screenshot 2023-01-31 at 19 30 05

Screenshot 2023-01-31 at 19 29 50

Screenshot 2023-01-31 at 19 33 46

@mtojek mtojek self-assigned this Jan 25, 2023
@mtojek mtojek mentioned this pull request Jan 25, 2023
Closed
BrunoQuaresma
BrunoQuaresma reviewed Jan 25, 2023
View reviewed changes
site/src/components/RichParameterInput/RichParameterInput.tsx
size="small"
disabled={disabled}
placeholder={parameter.default_value}
value={parameterValue}
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma Jan 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error usually happens when the value changes from string to undefined. Many times, having undefined values is a "bug" warning but in a few cases, it is ok so what you have to do is pass an empty string as a fallback:

value={parameterValue ?? ""}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I introduced the parameterValue as I wasn't able to fix it with formik. It looks like formik has troubles with understanding the concept of array of object, and for some reason, the input goes undefined. I will look into this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so maybe the getFormHelpers has a bug 🤔

@mtojek mtojek marked this pull request as ready for review January 31, 2023 18:33
@mtojek
Copy link
Member Author

mtojek commented Jan 31, 2023

Hey @BrunoQuaresma! I'm done with the implementation, but I need to increase test coverage for Update workspace. I will do this tomorrow, but I appreciate any comments on the code in the mean time. This should be reviewable.

PS. I apologize for the PR size...

@ammario ammario mentioned this pull request Jan 31, 2023
Closed
BrunoQuaresma
BrunoQuaresma reviewed Feb 1, 2023
View reviewed changes
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FE code looks very good to me, congrats! I'm going to run a QA before approving it.

BrunoQuaresma
BrunoQuaresma approved these changes Feb 1, 2023
View reviewed changes
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, if you already did that and you are confident, feel free to merge it!

@mtojek mtojek merged commit f9ae105 into coder:main Feb 1, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Feb 1, 2023
@bpmct bpmct added this to the Rich Parameters GA milestone Feb 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BrunoQuaresma BrunoQuaresma BrunoQuaresma approved these changes

@kylecarbs kylecarbs Awaiting requested review from kylecarbs

Assignees

@mtojek mtojek

Labels
None yet
Projects
None yet
Milestone
Rich Parameters GA
Development

Successfully merging this pull request may close these issues.
3 participants
@mtojek @BrunoQuaresma @bpmct