-
Notifications
You must be signed in to change notification settings - Fork 28
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
[MTV-2247] Initial new create plan wizard setup #1525
base: main
Are you sure you want to change the base?
Conversation
f0cd197
to
a27e320
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1525 +/- ##
==========================================
- Coverage 36.81% 36.14% -0.67%
==========================================
Files 158 157 -1
Lines 2548 2606 +58
Branches 599 633 +34
==========================================
+ Hits 938 942 +4
- Misses 1428 1662 +234
+ Partials 182 2 -180 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hey @jpuzz0 no need to nest the files under modules , we will omit the this folder. |
packages/forklift-console-plugin/src/modules/Plans/views/create/CreatePlanWizard.tsx
Outdated
Show resolved
Hide resolved
...forklift-console-plugin/src/modules/Plans/views/create/components/CreatePlanWizardFooter.tsx
Outdated
Show resolved
Hide resolved
...forklift-console-plugin/src/modules/Plans/views/create/components/CreatePlanWizardFooter.tsx
Outdated
Show resolved
Hide resolved
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.
@jpuzz0 great work!
I left some comments/ nit suggestions
packages/forklift-console-plugin/src/modules/Plans/views/create/CreatePlanWizard.tsx
Outdated
Show resolved
Hide resolved
packages/forklift-console-plugin/src/modules/Plans/views/create/CreatePlanWizard.tsx
Outdated
Show resolved
Hide resolved
packages/forklift-console-plugin/src/modules/Plans/views/create/PlanCreatePageV2.tsx
Outdated
Show resolved
Hide resolved
packages/forklift-console-plugin/src/modules/Plans/views/create/PlanCreatePageV2.tsx
Outdated
Show resolved
Hide resolved
packages/forklift-console-plugin/src/modules/Plans/views/create/PlanCreatePageV2.tsx
Outdated
Show resolved
Hide resolved
packages/forklift-console-plugin/src/modules/Plans/views/create/PlanCreatePageV2.tsx
Outdated
Show resolved
Hide resolved
...forklift-console-plugin/src/modules/Plans/views/create/components/CreatePlanWizardFooter.tsx
Outdated
Show resolved
Hide resolved
...ages/forklift-console-plugin/src/modules/Plans/views/create/steps/GeneralInformationForm.tsx
Outdated
Show resolved
Hide resolved
357f963
to
c1c1c83
Compare
packages/forklift-console-plugin/src/plans/create/CreatePlanWizard.tsx
Outdated
Show resolved
Hide resolved
packages/forklift-console-plugin/src/plans/create/CreatePlanWizardFooter.tsx
Outdated
Show resolved
Hide resolved
</WizardStep>, | ||
<WizardStep | ||
key={PlanWizardStepId.VirtualMachines} | ||
{...getStepProps(PlanWizardStepId.VirtualMachines)} |
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.
u are already introducing getStepProps, why not add a key prop there, also?
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.
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.
if you declare the type return from getStepProps that has key prop? still no go?
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.
packages/forklift-console-plugin/src/plans/create/CreatePlanWizard.tsx
Outdated
Show resolved
Hide resolved
packages/forklift-console-plugin/src/components/FormErrorHelperText.tsx
Outdated
Show resolved
Hide resolved
packages/forklift-console-plugin/src/plans/create/steps/GeneralInformationForm.tsx
Outdated
Show resolved
Hide resolved
packages/forklift-console-plugin/src/plans/create/steps/GeneralInformationForm.tsx
Outdated
Show resolved
Hide resolved
<TextInput | ||
{...field} | ||
validated={ | ||
errors[GeneralFormFieldId.PlanName] |
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.
do we need it? isnt this triggered automatically when next step is clicked?
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.
Yes, this validated
prop is what determined the text input's red outline. On "next" click, or any trigger of validation, the errors
will be updated to reflect any new entry, and the ID is what we're using to point to the correct error value here - to again - determine whether the input should have a red outline or not.
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 think if u add
error={Boolean(fieldState?.error)}
fieldState , is the second argument u can deconstruct, {field, fieldState}, we will get the same effect without the need for ternary check
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.
btw i think u can use helperText={fieldState?.error?.message}
and omit the formerrorhelpertext , please try
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.
Add error={Boolean(fieldState?.error)}
where exactly? This is not a prop that PF's TextInput
has, nor FormGroup
. And when I print fieldState inside of that callback for the render function, I just see an empty object even when an error is triggered.
Also, helperText
is not a prop that TextInput
or FormGroup
have.
Patternfly's example of using error helper text can be seen in their demos, which shows a similar implementation to what we have here:
https://www.patternfly.org/components/helper-text/react-demos#static-text-and-dynamic-status
1791333
to
7bd5db1
Compare
packages/forklift-console-plugin/src/plans/create/CreatePlanWizardFooter.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Jeff Puzzo <[email protected]>
7bd5db1
to
7c15c85
Compare
|
📝 Links
https://issues.redhat.com/browse/MTV-2247
📝 Description
Initial setup of the newer wizard which is mainly getting the framework of using react-hook-form and validation established between steps and demonstrating a single field's data is stored and submittable from the Review and create (last) step.
Accessible for testing via a new URL,
http://localhost:9000/mtv/create/plan
, which will have to be entered manually, as the actions to create migration plans throughout the plugin still point to the current wizard.🎥 Demo
Screen.Recording.2025-03-17.at.3.21.49.PM.mov
📝 CC://