feat: Save Generator Form State for a Session #185
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Laurin-W
force-pushed
the
feat/generator-ui/save-form-state
branch
from
02f92fc
to
32980f3
Compare
Laurin-W
force-pushed
the
feat/generator-ui/save-form-state
branch
from
32980f3
to
6c0134c
Compare
|
Heyho, I finally got some time, added an e2e test and refactored the code a bit. The form now uses a modified version of Happy for any feedback and improvement suggestions! I hope you have a good christmas time! |
There was a problem hiding this comment.
I'm a little surprised by this PR, but thanks! There are some issues with the safety of certain operations (json.parse and setItem), which should be fixed; Additionally I think it only makes sense to restore the state if it's within a given timeframe, instead of being indefinite, so you'd want to track a "last modified" value and compare that to see if it's current data to restore, otherwise this could potentially leak information.
e2e/generatorForm.playwright.ts
Outdated
|
|
||
| // Expect clientId error state to have remained. | ||
| expect( | ||
| await page.locator(`.MuiFormHelperText-root.Mui-error`).count() |
There was a problem hiding this comment.
This would be a good element to add a data-testid attribute to, potentially
There was a problem hiding this comment.
Changed in d5425f1.
src/components/useVerboseFormik.ts
Outdated
| ); | ||
| }; | ||
|
|
||
| return { ...modifiedFormik, addChild, removeChild }; |
There was a problem hiding this comment.
you'll want to useMemo here.
There was a problem hiding this comment.
I'm not quite sure how I would employ this here and I couldn't quite make sense of the documentation I found.
Wrapping the whole function makes the linter say: React Hook "useFieldStates" cannot be called inside a callback. React Hooks must be called in a React function component or a custom React Hook function.
If only wrapping parts, the linter comoplains: React Hook useMemo has missing dependencies
Could you give me a hint here?
src/components/useVerboseFormik.ts
Outdated
| * | ||
| * @returns formik object | ||
| */ | ||
| export default function useVerboseFormik<FormParameters extends FormikValues>({ |
There was a problem hiding this comment.
How's this "verbose" ? Perhaps extended?
There was a problem hiding this comment.
I used "verbose" since the extension allows the form to hold not only error but also warning, info and success messages and I used the "verbose" in VerboseTextField etc.
But I guess, it's not really that clear and I'll change it.
There was a problem hiding this comment.
Changed in 8770679
| @@ -87,10 +75,36 @@ export default function ClientIdentifierGenerator() { | |||
| defaultMaxAge: undefined, | |||
| }; | |||
|
|
|||
| /** Helper to load form state and values from storage or defaults. */ | |||
| const getSessionStorageStateOrDefault: () => { | |||
There was a problem hiding this comment.
it'd be an idea to add a max-age to this, such that coming back after a day doesn't restore data, it's really only for "I changed pages and oops I actually wanted that data"
There was a problem hiding this comment.
Addressed in ec0e242
src/pages/generate.tsx
Outdated
| const initialValues = storedFormValues | ||
| ? JSON.parse(storedFormValues) | ||
| : initialFormValues; | ||
| const initialStates = | ||
| formValidationState && storedFormValues | ||
| ? JSON.parse(formValidationState) | ||
| : getEmptyFormState(); |
There was a problem hiding this comment.
JSON.parse can throw, make sure you do this safely.
There was a problem hiding this comment.
Addressed in ec0e242.
src/pages/generate.tsx
Outdated
| // Save state of the form. | ||
| sessionStorage.setItem("formValues", JSON.stringify(formik.values)); | ||
| sessionStorage.setItem( | ||
| "formValidationState", | ||
| JSON.stringify(formik.states.all) | ||
| ); |
There was a problem hiding this comment.
These operations can throw, make sure they're handled safely.
There was a problem hiding this comment.
Addressed in ec0e242
| form.setFieldTouched(field, true) | ||
| formik.setFieldTouched(field, true) |
There was a problem hiding this comment.
The formik references call the object formik as well so I figured, it'd be better to stick to that convention.
* simplify formik state handling and add custom useFormik hook.
Laurin-W
force-pushed
the
feat/generator-ui/save-form-state
branch
from
6c0134c
to
ec0e242
Compare
|
Hey Laurin, sorry for the delay here, but it's going to take a while for us to get to this (it's been pushed into our backlog to review), I had been hoping to review this over christmas for you, but I caught covid and was out for 2 solid weeks. |
This PR makes the generator UI store its state with the
sessionStoreAPI so that when re-visiting the form after going to the validator, for example, the entered form values are not lost.This is rather a WIP since I will probably refactor a bit of the formik state management to make this less ugly.
TODO: