chore: remove classic parameters frontend code #20710
Conversation
| <DebouncedParameterField | ||
| id={id} | ||
| parameter={parameter} | ||
| value={value} | ||
| onChange={onChange} | ||
| disabled={disabled} | ||
| /> |
There was a problem hiding this comment.
Deboucing is quite important for input, textarea and slider parameters to allow the UI to remain responsive, if this is getting removed, how is debouncing being handled?
There was a problem hiding this comment.
I'm debouncing the websocket messages now directly instead of making that the fields jobs
having the fields not update the state immediately means that you have to wait half a second before you can actually hit enter/click create.
| return ( | ||
| <div | ||
| className="flex flex-col gap-2" | ||
| data-testid={`parameter-field-${parameter.name}`} |
There was a problem hiding this comment.
There are a few places that were changed to use display name to lookup a parameter but the data-testid here is still using name,
page.getByTestId( parameter-field-${richParameter.displayName}, );
There was a problem hiding this comment.
parameter.name here is, confusingly, the "displayName"
There was a problem hiding this comment.
Most excellent 👌
One main question and some observations from messing around with it (that are probably out of scope, and you may have already have them written down):
- Do we need to do anything with the
use_classic_parameter_flowsetting before merging this? Dynamic parameters have to be enabled in the template settings, but then when creating a workspace it uses dynamic parameters regardless of the setting. But then other spots can still use the classic flow, like the ephemeral build popup, or the/settings/parameterspage. - One weird thing (I bet this is a preexisting issue), if I have a number input and I type text into it, I get no error feedback. If I type a number then erase it I get
"" is not a number, but then if I type again the error still says""and not the actual input I wrote. Maybe the bigger issue is that Firefox lets me type any text into a number field though lol. - I know this is an preexisting issue and out of scope for this PR, but I did want to mention it feels kinda weird when the
Requiredbadge disappears after filling out the field, and it does not come back after removing the value either. Since we already show a*when the field is required, maybe we show the badge instead of*and remove the logic that currently toggles the badge.
| if (validation_max !== null) { | ||
| inputProps.max = validation_max; | ||
| } | ||
| } else if (parameter.styling?.mask_input) { |
There was a problem hiding this comment.
nbd at all but could make sense to use the var that was derived
| } else if (parameter.styling?.mask_input) { | |
| } else if (maskInput) { |
| import type { AutofillBuildParameter } from "utils/richParameters"; | ||
| import { paramsUsedToCreateWorkspace } from "utils/workspace"; | ||
| import { CreateWorkspacePageView } from "./CreateWorkspacePageView"; | ||
| import { CreateWorkspacePageViewExperimental } from "./CreateWorkspacePageViewExperimental"; |
There was a problem hiding this comment.
Should we call this just CreateWorkspacePageView now?
| type ExternalAuthPollingState = "idle" | "polling" | "abandoned"; | ||
|
|
||
| const CreateWorkspacePage: FC = () => { | ||
| const CreateWorkspacePageExperimental: FC = () => { |
There was a problem hiding this comment.
Similarly here, should this just be CreateWorkspacePage now?
| }; | ||
|
|
||
| export default CreateWorkspacePage; | ||
| export default CreateWorkspacePageExperimental; |
There was a problem hiding this comment.
Keep the name as CreateWorkspacePage
| <Loader /> | ||
| ) : ( | ||
| <CreateWorkspacePageView | ||
| <CreateWorkspacePageViewExperimental |
There was a problem hiding this comment.
Keep the name as CreateWorkspacePageView
|
|
|
Just for continuity if we ever look at the PR sometime in the future it may be helpful to link to issues for remaining work, bugs that are still needed. For example, removing classic parameters for the WorkspaceParametersPage.tsx |
jaaydenh
left a comment
There was a problem hiding this comment.
After testing, I didn't discover any new issues. Left some minor comments but otherwise should be good to go.
f51c0dc to
782c6fa
Compare
|
Closes coder/internal#1075 |
Delete all of the old frontend code and fix all of the tests that broke in the process
I found a number of quirks and bugs that we should probably address, but I think this is a solid chunk of work to merge