Form submission refactoring #203

Bernardstanislas · 2021-05-11T16:13:34Z

No description provided.

rdubigny

I am really fond of the splitting in several little files, particularly of the separated button configuration file with explicit behavior. This implementation is simpler on many aspects. Thanks.

rdubigny · 2021-05-18T12:13:48Z

package-lock.json

@@ -3188,6 +3188,31 @@
        "num2fraction": "^1.2.2",
        "postcss": "^7.0.32",
        "postcss-value-parser": "^4.1.0"
+      },
+      "dependencies": {


I have doubt that these changes are due to using the wrong version of npm.

Could you confirm this is not the case ?

rdubigny · 2021-05-18T12:14:27Z

package.json

@@ -4,7 +4,7 @@
  "description": "",
  "main": "index.js",
  "engines": {
-    "node": ">=12.18.3"
+    "node": ">=12.18.3 <14"


actually the right not version is : ~12.18.3

rdubigny · 2021-05-18T12:30:32Z

src/components/templates/Form/SubmissionPanel/index.js

+  const showPrompt = pendingAction !== null;
+
+  const confirmPrompt = useRef();
+  const cancelPrompt = useRef();


why not use react context ? if the point is to use a shared state it seems the right way to do it. What am I missing ?

rdubigny · 2021-05-18T12:38:25Z

src/components/atoms/FormActionButton.js

+    setLoading(true);
+
+    const userInteractionPromise = new Promise((resolve, reject) => {
+      confirmPrompt.current = resolve;


In my opinion, this kind of logic should be handled in the SubmissionPanel component, not in an atom.

rdubigny · 2021-05-18T12:46:45Z

src/components/molecules/FormActionButtonList.js

+        return (
+          <FormActionButton
+            key={actionConfiguration.id}
+            actionConfiguration={actionConfiguration}


I'd rather set explicitly label, icon and cssClass explicitly than passing an opaque object

rdubigny · 2021-05-18T12:49:43Z

src/components/molecules/FormActionButtonList.js

+            actionConfiguration={actionConfiguration}
+            loading={loading}
+            setLoading={setLoading}
+            {...props}


I am not found of passing an opaque amount of props to an atoms. This comment is linked to the complex logic embarked in the onClick on the atomic button.

rdubigny · 2021-05-18T12:59:44Z

src/components/atoms/FormActionButton.js

+      confirmPrompt.current = resolve;
+      cancelPrompt.current = reject;
+    });
+    const resultMessages = await handleEnrollmentSubmission(


I do not agree with handling the most important logic of the whole SubmissionPanel in an atom.

Maybe this should be solved simply by defining the onClick behavior in the SubmissionPanel and passing this function to children.

rdubigny · 2021-05-18T13:05:15Z

src/lib/enrollment-submission-handler.js

+      await deleteEnrollment({ id: enrollmentId });
+    }
+
+    if (actionConfiguration.needsToComputeNextEnrollmentState) {


I like the fact that this is now a parameters, but I can't manage to understand the meaning of this. Can we talk about this ?

Maybe my confusion is partially due to NextEnrollment being a name used elsewhere in the codebase.

rdubigny

fantastic work !

Bernardstanislas changed the title ~~copy form~~ WIP - Copy form button + form submission refactoring May 17, 2021

Bernardstanislas force-pushed the copy-form branch 2 times, most recently from f0804bc to 30f55cf Compare May 17, 2021 14:23

Bernardstanislas changed the title ~~WIP - Copy form button + form submission refactoring~~ Form submission refactoring May 17, 2021

rdubigny suggested changes May 18, 2021

View reviewed changes

Bernardstanislas added 25 commits May 19, 2021 14:24

refacto: extract static variables to test them

ab1ec1c

chore: Set max node version since lock is in v1 format

cb49e61

refacto: extract submit function handler factory

a04de22

refacto: Extract render function

9307c09

refacto: Move state modification out of unclear function

d5ab139

refacto: Rename wait user interaction function

18ae942

refacto: Rename prompt callbacks

bacfb59

refacto: Give buttons params generation to render function

997a555

refacto: Purify handler factory

dda3135

refacto: Move handler factory to render function

c47e68a

refacto: Temp render is now a component

5bf78ee

refacto: Move show prompt state

461b442

refacto: Intended action is now a function state

a7f6fb0

refacto: Purify trigger action function

6b70196

refacto: Move trigger action to its own file

e611dd1

refacto: Pure function

4d1d1a5

chore: Reduce npm audit warnings

3f33373

chore: Update dependencies

45df0a1

refacto: Fix filename case

1019ab3

refacto: Move hook to a hook folder

8b4ad45

refacto: Extract button to its own file

99b2685

refacto: Extract button list to a molecule

1b69361

refacto: Use atom action button

8365dfa

refacto: Rename whole zone to submission panel

2c7a5d6

refacto: Move buttons listing logic to button list

87add66

Bernardstanislas added 2 commits May 19, 2021 14:25

chore: test destroy action

5bc6d02

chore: Test update action

146667d

Bernardstanislas force-pushed the copy-form branch from 66a1796 to 146667d Compare May 19, 2021 12:26

Bernardstanislas added 20 commits May 19, 2021 14:37

chore: Move jest config to its own section

1249fbd

chore: Switch App.js to typescript

51b5c86

chore: Move dev dependencies to dev section

6d75605

feat: Empty form submission hook

32e59d9

feat: Loading state

bf6e59f

feat: Hook manages pending action

f64ff1c

feat: Compute action configuration

753038e

feat: Button click handler

5d5e1eb

refacto: Simplify mocking

060c4dd

lint: Lint using eslint

8dce98a

refacto: Typescript submission handler

7581650

feat: Set pending action on button click

df3befb

feat: Call handleSubmit with user inputs

7b03627

recycle: Loading state is computed from pending action

ea67563

test: Check action behaviour

348b1e0

test: New action handler

0501baf

refacto: Separate display props

6bec9c2

refacto: Use new hook

e9a8de1

refacto: Remove old function

ef23f94

test: Prompt cancellation logic

8f305f2

Bernardstanislas force-pushed the copy-form branch from 3904cc5 to 8f305f2 Compare May 20, 2021 09:24

Bernardstanislas added 3 commits May 20, 2021 11:27

feat: Separate CI tasks

21c7a7c

linting: Fix service worker linting

9ffaeb2

test: Setup circleci workflow

3898688

rdubigny approved these changes May 20, 2021

View reviewed changes

rdubigny merged commit c3d8e2f into master May 20, 2021

rdubigny deleted the copy-form branch May 20, 2021 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Form submission refactoring #203

Form submission refactoring #203

Bernardstanislas commented May 11, 2021

rdubigny left a comment

rdubigny May 18, 2021

rdubigny May 18, 2021

rdubigny May 18, 2021

rdubigny May 18, 2021

rdubigny May 18, 2021

rdubigny May 18, 2021

rdubigny May 18, 2021

rdubigny May 18, 2021

rdubigny May 18, 2021

rdubigny left a comment

Form submission refactoring #203

Form submission refactoring #203

Conversation

Bernardstanislas commented May 11, 2021

rdubigny 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

rdubigny left a comment

Choose a reason for hiding this comment