-
Notifications
You must be signed in to change notification settings - Fork 597
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
feat: implemented multi step form #3436
Conversation
Functionality is ready. |
I have several questions regarding Form behaviour: 2 - Let's say that we have created 4 form step, then we swithced to the preview tab, navigated to the last step and we decided to delete last step and the question is, what should we do with the preview, should we reset all fields values and reset preview step to the first step in the list, or should we just reset preview step to the first step? As an example below now we reset our preview to the first step, whenever we remove step. 2023-07-21.14.03.02.mov |
@adrians5j what should we do about migration? Should I implement it? Or someone else gonna do that? @SvenAlHamad & @adrians5j also I need your opinion on my comment above #3436 (comment) |
ede2b2a
to
128f4e3
Compare
Hey @neatbyte-ivelychko, thanks for the questions. Regarding how many fields in a row...I can see the current limit is 4 fields per row, plus on a wider screen, it looks fine. But, if it's not too much trouble, we can add this so we have a horizontal scrollbar on smaller screens: For now I guess it's fine. Regarding the form reset, having the form completely reset or resetting to step 1 is both fine at this point I guess. |
Leave the migration to us BTW. |
@adrians5j okay, got it. |
@adrians5j I have an idea - for forms import we can add support for OLD exports (without new It will be almost same code as an migration - just not for db records, but for import archive records |
@adrians5j also what should we do with cypress tests, should we add more of them in this PR (I think that we need to add tests for steps and also fix already existing if they fail) or we will do that in another PR? |
I did a quick test on this one and have some feedback:
|
Cypress E2E tests have been initiated (for more information, click here). ✨ |
/cypress |
41209f1
to
9e211d9
Compare
Also I did a quick research on how to handle drag-n-drop in Cypress |
packages/app-form-builder/src/admin/components/FormEditor/Context/functions/getFieldPosition.ts
Outdated
Show resolved
Hide resolved
console.log(source); | ||
return; | ||
} | ||
const fieldId = (pos.formStep.id !== formStep.id ? pos.formStep : formStep).layout[ |
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.
Will add a comment that would explain what are we doing here
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 point.
9e211d9
to
3ab0461
Compare
Sorry to ask, this PR can be reviewed? Re migrations, basically, since the form data structure will become different once this PR is merged, we'll need to write a migration script that upgrades all existing form entries in the DB, so that existing data conforms with the new code. But we'll handle that. That's why I added You did mention import/exports though, and that's a good point. If it's not like a week of work, it would certainly be good to be able to import forms created with older version of Webiny into the latest version. So yeah, feel free to check that one one and ping for any further questions. Finally, just a note on Cy tests. We've decided we'll be taking over here, so, feel free to finish any WIP Cy tests, but no need to create new ones from now on. |
@adrians5j Yeah, it is ready to be reviewed |
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.
Did the first review, managed to go over like ~60% of files.
Overall looking good, but yeah, there's always stuff to polish / improve.
Please check my comments and let me know if there will be any questions.
Once we deal with this first batch of feedback, I'll continue with the rest.
// All form fields - an array of rows where each row is an array that contain fields. | ||
const fields = getFields(); | ||
// @ts-ignore |
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.
We'll want to check this ts-ignore here.
import { Form } from "@webiny/form"; | ||
import React, { useState, useEffect } from "react"; | ||
import { Form, FormAPI } from "@webiny/form"; | ||
import { ButtonDefault, ButtonPrimary } from "@webiny/ui/Button"; |
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.
Basically, the form layout was imagined as an as lightweight as possible code. Meaning, we had SubmitButton
in there, totally coded from scratch and not using @webiny/ui
package.
We need to continue the same approach here.
setCurrentStep(prevStep => (prevStep += 1)); | ||
}; | ||
|
||
const handlePrevStep = () => { |
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.
We need to handle step back / step fwd controls and state internally, within the app-form-builder package's component. The same thing we're doing for example with submit
, pulled from FormLayoutComponent
. So, let's add those controls above.
The idea is to have these FB layouts as straightforward as possible, and be able to provide as much of OOTB functionality as possible. With this approach, if the user wanted to create another layout, they'd need to c/p again the steps state management code.
console.log(source); | ||
return; | ||
} | ||
const fieldId = (pos.formStep.id !== formStep.id ? pos.formStep : formStep).layout[ |
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 point.
packages/app-form-builder/src/admin/components/FormEditor/Context/functions/getFieldPosition.ts
Outdated
Show resolved
Hide resolved
@adrians5j I've implemented first part of suggested changes. |
/cypress |
Cypress E2E tests have been initiated (for more information, click here). ✨ |
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.
Another iteration done. Thanks for the work you did here so far.
With code comments, here are a couple of comments related to UI / manual testing I did.
1.Styling of "Steps" In Editor
Should we adjust the styling here a bit @SvenAlHamad ? I'm thinking the same thing we have in our CMS? Check sshot.
In our case, this could be the step name.
2. Step Name Modal Improvements
Please check sshot.
3. Add Step Button
Not sure how to improve here.. IDK, but it looks a bit weird. Maybe @SvenAlHamad has an idea how to improve this?
packages/app-page-builder-elements/src/renderers/form/FormRender.tsx
Outdated
Show resolved
Hide resolved
// Validate fields for current step with "form.validateInput" function, | ||
// if current step is invalid then we should block posibility to move to the next step, | ||
// but if step is valid then user can switch to the next step. | ||
const validateCurrentStepFields = (form: FormAPI) => { |
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.
Let's also have this as an internal function users can immediately use.
But one thing though... When you call submit
(the one passed by the Form
component, L122), the rendered fields will be automatically validated. And then the Form
component only calls your onSubmit
callback if the validation is successful.
In other words, the validation of fields should probably be:
- handled by the @webiny/app-form-builder's Form component internally.
- should be internally handled by the
submit
callback / the @webiny/form package which triggers it automatically for us.
Hope I didn't miss anything here.
// we will simpy change currentStep to the first step. | ||
useEffect(() => { | ||
setCurrentStep(0); | ||
}, [data.steps]); |
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.
To be more precise, we could also do data.steps.length
here, right?
const getFields = (): FormRenderFbFormModelField[][] => { | ||
const fieldLayout = cloneDeep(layout); | ||
// We need to have "stepIndex" prop in order to get corresponding fields for the current step. | ||
const getFields = (stepIndex: number): FormRenderFbFormModelField[][] => { |
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.
Let's have stepIndex = 0
here, just so if a user knows he's working with single-step forms, he can just call getFields()
and that's it.
await updateRevision({ revision: id, data: { fields } }); | ||
await updateRevision({ | ||
revision: id, | ||
data: { fields, steps: [{ title: "", layout: [] }] } |
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.
Seeing this test here, not only it might be better to have a title here just saying "Test step" at lest, but also maybe have validation that ensures title is not an empty string. Can we try to see if this is easily doable and if so, can we do it?
@@ -170,6 +187,12 @@ describe("Forms Submission Security Test", () => { | |||
...formA, | |||
savedOn: expect.stringMatching(/^20/), | |||
fields: expect.any(Array), | |||
steps: [ | |||
{ | |||
title: null, |
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.
Confused a bit why we sometimes have ""
, and sometimes null
here. Am I missing something here?
But in any case, if we add the "required" validator I mentioned in my prev comments, this should no longer be an issue.
@SvenAlHamad could you please take a look at these comments #3436 (review) |
Tip: a couple of UI-related questions for you @SvenAlHamad above. |
@SvenAlHamad please take a look on the questions above |
With regards to the UI questions above on #1 and #2 I agree with Adrian's suggestions. As for #3, I would just switch places between the icon and the text, so the text is on the right and icon on the left. This is similar to the design we have for dynamic zone in HCMS, so I'm ok with keeping this style instead of adding a button. |
@adrians5j I have pushed the latest requested changes |
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.
@adrians5j Maybe you were moving row instead of field? Because we cannot drop rows in rows. Here is the demo: |
306771a
to
5eccf49
Compare
@adrians5j so after merging latest next, api test for form builder are failing because of prerendering, should we fix them right now? Because before merging they were working |
It's an issue that was introduced by the FB prerendering PR yes. We both missed failing API tests before merging. I'll see now if I can take care of it quickly. |
Co-authored-by: adrians5j <adrian@webiny.com> Co-authored-by: Adrian Smijulj <adrian1358@gmail.com>
Changes
Implements issue "Add the ability to build multi-step forms".
How Has This Been Tested?
Manual
Documentation
None