feat: Refactor onDrop for Multi Step Form #3574

neatbyte-vnobis · 2023-10-04T09:29:32Z

Changes

Refactored the onDrop function for Fields, Rows and Condition Groups.

How Has This Been Tested?

Manually

Documentation

https://www.notion.so/Proposal-to-refactor-the-onDrop-function-for-Fields-Rows-and-Condition-Groups-a3c3604f42d4495eb4bc9f5f4774dfdf

neatbyte-ivelychko · 2023-10-04T09:31:26Z

@adrians5j
The refactor of onDrop for Multi Step Form is done.
Could you please review it?

neatbyte-ivelychko · 2023-10-05T10:32:08Z

@adrians5j I don't know whether it is okay that property _id of form field can be undefined.
Because if we want to pass that prop in some functions we would need to add extra check whether that id exists, and it is kinda strange. Or it a desired behaviour of _id property?

https://github.com/webiny/webiny-js/blob/next/packages/app-form-builder/src/types.ts#L195

packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/FormStep/FormStep.tsx

adrians5j · 2023-10-18T12:49:53Z

Re ?_id, I think it makes sense to have it as a required field. Especially because on the GQL level, I can see it's non-nullable:

If you want, we can try updating this and see if anything breaks.

neatbyte-ivelychko · 2023-10-18T12:53:44Z

If you want, we can try updating this and see if anything breaks.

I think I would try to update it.

packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/FormStep/FormStep.tsx

… params

packages/app-form-builder/src/admin/components/FormEditor/Context/useFormEditorFactory.tsx

neatbyte-ivelychko · 2023-10-23T07:37:27Z

@adrians5j could you please review this pr?

adrians5j

Initial review done. Didn't go over the whole PR, but it's a good start I'd say.

BTW I applaud the comments 👏🏻 . I just added a couple of directions.

P.S. I added two commits.

packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/FormStep/FormStep.tsx

packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/EditTab.tsx

packages/app-form-builder/src/admin/components/FormEditor/Droppable.tsx

packages/app-form-builder/src/admin/components/FormEditor/Draggable.tsx

packages/app-form-builder/src/admin/components/FormEditor/Context/functions/moveStep.ts

packages/app-form-builder/src/admin/components/FormEditor/Context/functions/moveRowBetween.ts

packages/app-form-builder/src/admin/components/FormEditor/Context/functions/moveRow.ts

neatbyte-ivelychko · 2023-11-27T08:34:21Z

@adrians5j Hi, I have made all of the requested changes. Could you please take a look?

packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/FormStep/FormStep.tsx

packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/EditTab.tsx

packages/app-form-builder/src/hooks/useFormDragAndDrop.ts

adrians5j

Did some changes, left some comments.

Please check it out when you get a chance.

packages/app-form-builder/src/admin/components/FormEditor/Context/useFormEditorFactory.tsx

packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/FormStep/FormStep.tsx

packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/FormStep/useFormStep.ts

packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/Styled.ts

packages/app-form-builder/src/admin/components/FormEditor/Context/useFormEditorFactory.tsx

packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/FormStep.tsx

packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/EditTab.tsx

adrians5j · 2023-12-13T12:39:26Z

Hm, okay, just so you're unblocked. But, I'd still see why this is happening. It's probably just a missing dependency in the array. Can you double check? If no luck, then yeah, feel free to proceed.

…

On Wed, 13 Dec 2023 at 12:31, Illya Velychko ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/EditTab.tsx <#3574 (comment)>: > }; // This function will render drop zones on the top of the step, // if steps are locatted above "source" ("source" step is the step that we move). - const renderTopDropZone = (sourceStepId: string | undefined, targetStepId: string) => { - if (!sourceStepId) { + const renderTopDropZone = (item: IsVisibleCallableParams, targetStepId: string) => { @adrians5j <https://github.com/adrians5j> Also I have noticed that if we add a useCallback or useMemo methods from useFormStep stops working properly. For instance moveRow does not work properly, it would only move one field and delete another field instead of moving an entire row into another step. And it is only one of many bugs that occurs after using useCallback. And I just want to know whether it is okay that we would not use useCallback and useMemo? — Reply to this email directly, view it on GitHub <#3574 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABHCI7FVGSFZN7CYOWVYES3YJGGZTAVCNFSM6AAAAAA5SJ3ER2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONZZGQZTIMZVGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Adrian S.

neatbyte-ivelychko · 2023-12-13T15:01:25Z

@adrians5j

I've refactored EditTab, and fixed issues with useCallback.
Could you please review it again?

...er/src/admin/components/FormEditor/Tabs/EditTab/FormStep/FormStepContext/FormStepContext.tsx

packages/app-form-builder/src/hooks/useFormDragAndDrop.ts

...der/src/admin/components/FormEditor/Tabs/EditTab/FormStep/FormStepWithFields/FormStepRow.tsx

.../app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/EditFieldDialog/useEditTab.ts

packages/app-form-builder/src/hooks/useFormDragAndDrop.ts

packages/app-form-builder/src/admin/components/FormEditor/Droppable.tsx

packages/app-form-builder/src/admin/components/FormEditor/Draggable.tsx

packages/app-form-builder/src/admin/components/FormEditor/Context/useFormEditorFactory.tsx

adrians5j

Almost there!

adrians5j · 2023-12-19T10:52:45Z

packages/app-form-builder/src/types.ts

+export interface MoveStepParams {
+    target: {
+        containerId: string;
+        position: {


Does it make sense to use DropTarget here?

adrians5j · 2023-12-19T10:53:03Z

packages/app-form-builder/src/types.ts

@@ -112,6 +145,19 @@ export interface FbFormStep {
    layout: FbFormModelFieldsLayout;
 }

+export interface MoveStepParams {
+    target: {


Does it make sense to rename this to source, the same way we have it above?

adrians5j · 2023-12-19T10:53:35Z

packages/app-form-builder/src/types.ts

+
+export interface DropSource {
+    // Contains info about the Container from which we are dragging an element or elements.
+    // containerId and containerType could be null in case we are creating a custom field.


Says here containerId and containerType can be null, but type doesn't reflect that. Maybe you meant undefined?

adrians5j · 2023-12-19T10:54:06Z

packages/app-form-builder/src/types.ts

+    // Contains info about the Container,
+    // in which we are dropping an element or elements.


Suggested change

// Contains info about the Container,

// in which we are dropping an element or elements.

// Contains info about the Container into which we are dropping one or more elements.

adrians5j · 2023-12-19T10:54:45Z

packages/app-form-builder/src/types.ts

+    // Property "id" is optional,
+    // because when we move row it does not have an id.


Suggested change

// Property "id" is optional,

// because when we move row it does not have an id.

// Property "id" is optional because when we move row it does not have an `id`.

adrians5j · 2023-12-19T10:55:42Z

packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/FormStep/useFormStep.ts

+    }, []);
+
+    const onFormStepDrop = useCallback(
+        (params: HandleDropParams) => {


Suggested change

(params: HandleDropParams) => {

(params: OnFormStepDropParams) => {

Also, does it make sense to to maybe just undo the name change here to handleDrop since we don't have the original one anymore?

adrians5j · 2023-12-19T11:05:01Z

packages/app-form-builder/src/admin/components/FormEditor/Droppable.tsx

@@ -20,29 +20,34 @@ export interface DroppableCollectedProps {
    isOver: boolean;
 }

+export type DroppableUIElements = "row" | "field" | "conditionGroup" | "step";


Suggested change

export type DroppableUIElements = "row" | "field" | "conditionGroup" | "step";

export type DraggableUIElement = "row" | "field" | "conditionGroup" | "step";

Notice the singular instead of plural.

P.S. At this point, ui name of the prop does sound ambiguous, but let's leave it as is for now (I'm aware if was like this before you started working on this).

adrians5j · 2023-12-19T11:05:16Z

packages/app-form-builder/src/admin/components/FormEditor/Droppable.tsx

+    id?: string;
+    pos: FieldLayoutPositionType;
+    container?: {
+        type: "step" | "conditionGroup";


Let's also have a type for this, e.g. ContainerType.

adrians5j · 2023-12-19T11:07:48Z

packages/app-form-builder/src/admin/components/FormEditor/Draggable.tsx

+    id?: string;
+    // "container" contains info about source element.
+    container?: {
+        type: "step" | "conditionGroup";


Again a use case where we could use a type that could live in types.ts file. Let's do that and also make sure to use the type in packages/app-form-builder/src/admin/components/FormEditor/Droppable.tsx.

adrians5j · 2023-12-19T11:44:02Z

.../app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/EditFieldDialog/useEditTab.ts

+        [data]
+    );
+
+    // This function will render drop zones on the top of the step,


Suggested change

// This function will render drop zones on the top of the step,

// This function will render drop zones on the bottom of the step,

neatbyte-ivelychko · 2023-12-20T08:59:20Z

@adrians5j fixed latest comments

adrians5j

One question.

adrians5j · 2023-12-20T21:12:38Z

packages/app-form-builder/src/types.ts

@@ -11,6 +11,44 @@ import {
 import { ApolloClient } from "apollo-client";
 import { SecurityPermission } from "@webiny/app-security/types";

+export type DropTargetType = "field" | "row" | "conditionGroup" | "step";


Does it make sense to use this instead of DroppableUIElement in packages/app-form-builder/src/admin/components/FormEditor/Droppable.tsx:23 ?

adrians5j · 2023-12-21T11:13:02Z

/cypress

github-actions · 2023-12-21T11:13:15Z

Cypress E2E tests have been initiated (for more information, click here). ✨

neatbyte-ivelychko reviewed Oct 5, 2023

View reviewed changes

packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/FormStep/FormStep.tsx Outdated Show resolved Hide resolved

neatbyte-vnobis mentioned this pull request Oct 5, 2023

Multi Step Form Rules and Condition Group #3577

Closed

3 tasks

neatbyte-vnobis changed the base branch from next to neatbyte/multi-step-form-rules-condition-group October 10, 2023 14:41

neatbyte-vnobis changed the base branch from neatbyte/multi-step-form-rules-condition-group to next October 10, 2023 14:44

neatbyte-vnobis changed the base branch from next to neatbyte/multi-step-form-rules-condition-group October 12, 2023 08:29

neatbyte-vnobis changed the base branch from neatbyte/multi-step-form-rules-condition-group to next October 12, 2023 08:30

neatbyte-vnobis force-pushed the neatbyte/multi-step-form-drag-n-drop branch from a5d10c7 to 1b8aaf6 Compare October 12, 2023 08:30

adrians5j self-assigned this Oct 18, 2023

neatbyte-ivelychko reviewed Oct 18, 2023

View reviewed changes

packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/FormStep/FormStep.tsx Outdated Show resolved Hide resolved

neatbyte-vnobis added 4 commits October 19, 2023 11:31

feat: refactored onDrop method for multi step form

39565e3

feat: improved destination property for inserting custom fields

3a12011

feat: removed HandleDrop interface and impoved comments

77b7006

fix: fixed type for field _id and created compose function for onDrop…

ef7b3b7

… params

neatbyte-vnobis force-pushed the neatbyte/multi-step-form-drag-n-drop branch from eb55a5c to ef7b3b7 Compare October 19, 2023 08:31

neatbyte-ivelychko reviewed Oct 19, 2023

View reviewed changes

packages/app-form-builder/src/admin/components/FormEditor/Context/useFormEditorFactory.tsx Outdated Show resolved Hide resolved

Merge branch 'next' into neatbyte/multi-step-form-drag-n-drop

96d6bee

adrians5j requested changes Nov 20, 2023

View reviewed changes

adrians5j and others added 6 commits November 20, 2023 11:03

refactor: group files into folders

84421e9

chore: format comments

aedc45e

fix: created hook for drag and drop, formated comments.

0efea36

fix: merged with next

0543b44

Merge branch 'next' into neatbyte/multi-step-form-drag-n-drop

4ba3ae0

Merge branch 'next' into neatbyte/multi-step-form-drag-n-drop

1f5e2c1

neatbyte-vnobis added the umbrella ☂️ label Nov 22, 2023

neatbyte-vnobis removed the umbrella ☂️ label Nov 28, 2023

adrians5j reviewed Nov 28, 2023

View reviewed changes

adrians5j added 5 commits November 28, 2023 17:24

refactor: create FormStepProps interface

7d42a0b

refactor: split FormStep into multiple files

03cfaad

refactor: split FormStepWithFields into multiple files

28da017

refactor: remove redundant props

3c248bc

refactor: remove old file

cd4b300

adrians5j requested changes Nov 29, 2023

View reviewed changes

neatbyte-vnobis force-pushed the neatbyte/multi-step-form-drag-n-drop branch 2 times, most recently from 1454c85 to cd4b300 Compare December 13, 2023 14:07

neatbyte-vnobis and others added 3 commits December 13, 2023 16:14

Merge branch 'next' into neatbyte/multi-step-form-drag-n-drop

c22fe22

fix: refactored EditTab

aacf52d

fix: added useCallback and useMemo for drag and drop methods

536ce7d

Merge branch 'next' into neatbyte/multi-step-form-drag-n-drop

27fc3d9

adrians5j requested changes Dec 18, 2023

View reviewed changes

fix: fixed dependency for callbacks, fixed types

f14cd5e

adrians5j requested changes Dec 19, 2023

View reviewed changes

neatbyte-vnobis and others added 2 commits December 19, 2023 14:52

fix: renamed onDrop function, improved imports

a647dd7

Merge branch 'next' into neatbyte/multi-step-form-drag-n-drop

a49134d

Merge branch 'next' into neatbyte/multi-step-form-drag-n-drop

51c8a57

adrians5j requested changes Dec 20, 2023

View reviewed changes

neatbyte-vnobis and others added 2 commits December 21, 2023 10:05

fix: removed useless type

c66dd15

Merge branch 'next' into neatbyte/multi-step-form-drag-n-drop

3ab94f7

adrians5j merged commit 7f8da60 into next Dec 21, 2023
78 checks passed

adrians5j deleted the neatbyte/multi-step-form-drag-n-drop branch April 5, 2024 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Refactor onDrop for Multi Step Form #3574

feat: Refactor onDrop for Multi Step Form #3574

neatbyte-vnobis commented Oct 4, 2023 •

edited

Loading

neatbyte-ivelychko commented Oct 4, 2023

neatbyte-ivelychko commented Oct 5, 2023

adrians5j commented Oct 18, 2023

neatbyte-ivelychko commented Oct 18, 2023 •

edited

Loading

neatbyte-ivelychko commented Oct 23, 2023

adrians5j left a comment •

edited

Loading

neatbyte-ivelychko commented Nov 27, 2023

adrians5j left a comment

adrians5j commented Dec 13, 2023 via email

neatbyte-ivelychko commented Dec 13, 2023

adrians5j left a comment

adrians5j Dec 19, 2023

adrians5j Dec 19, 2023

adrians5j Dec 19, 2023

adrians5j Dec 19, 2023

adrians5j Dec 19, 2023

adrians5j Dec 19, 2023

adrians5j Dec 19, 2023

adrians5j Dec 19, 2023

adrians5j Dec 19, 2023

adrians5j Dec 19, 2023

neatbyte-ivelychko commented Dec 20, 2023

adrians5j left a comment

adrians5j Dec 20, 2023

adrians5j commented Dec 21, 2023

github-actions bot commented Dec 21, 2023

		// Contains info about the Container,
		// in which we are dropping an element or elements.

	// Contains info about the Container,
	// in which we are dropping an element or elements.
	// Contains info about the Container into which we are dropping one or more elements.

		// Property "id" is optional,
		// because when we move row it does not have an id.

	// Property "id" is optional,
	// because when we move row it does not have an id.
	// Property "id" is optional because when we move row it does not have an `id`.

	(params: HandleDropParams) => {
	(params: OnFormStepDropParams) => {

	export type DroppableUIElements = "row" \| "field" \| "conditionGroup" \| "step";
	export type DraggableUIElement = "row" \| "field" \| "conditionGroup" \| "step";

	// This function will render drop zones on the top of the step,
	// This function will render drop zones on the bottom of the step,

feat: Refactor onDrop for Multi Step Form #3574

feat: Refactor onDrop for Multi Step Form #3574

Conversation

neatbyte-vnobis commented Oct 4, 2023 • edited Loading

Changes

How Has This Been Tested?

Documentation

neatbyte-ivelychko commented Oct 4, 2023

neatbyte-ivelychko commented Oct 5, 2023

adrians5j commented Oct 18, 2023

neatbyte-ivelychko commented Oct 18, 2023 • edited Loading

neatbyte-ivelychko commented Oct 23, 2023

adrians5j left a comment • edited Loading

Choose a reason for hiding this comment

neatbyte-ivelychko commented Nov 27, 2023

adrians5j left a comment

Choose a reason for hiding this comment

adrians5j commented Dec 13, 2023 via email

neatbyte-ivelychko commented Dec 13, 2023

adrians5j left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neatbyte-ivelychko commented Dec 20, 2023

adrians5j left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrians5j commented Dec 21, 2023

github-actions bot commented Dec 21, 2023

neatbyte-vnobis commented Oct 4, 2023 •

edited

Loading

neatbyte-ivelychko commented Oct 18, 2023 •

edited

Loading

adrians5j left a comment •

edited

Loading