Make modals/forms more reactive #5897

wolflu05 · 2023-11-10T10:21:08Z

This PR aims to make the modals and forms framework more reactive.

Separates ApiForms from modals so that they can be used on their own
Makes it possible to hook into the modal state and build some custom reactive things

This is already working, but it is a bit slow. I think this is caused by multiple reasons. The biggest is, that Form fields get passed every form state as props. That means they rerender on every change. But actually that is not needed. Because e.g. changing the value of a text input should not rerender the part category select input. The part category select input should only rerender if its definition changes. (This can be easily seen by placing console.log("Rerender", definition.label) inside of the ApiFormField)

EDIT: I now discovered, that the preview production build doesn't has that much of performance issues, and checking out the latest master in development has even more performance problems on my side.

Todo

Give feedback

fix `onValueChange` issue

fix onValueChange issue
Options
Successfully updated the issue's project

There was an error updating the issue's project
migrate to react-hook-form

migrate to react-hook-form
Options
Successfully updated the issue's project

There was an error updating the issue's project
improve initial data fetching query

improve initial data fetching query
Options
Successfully updated the issue's project

There was an error updating the issue's project
Loading existing value into Related model field does not work

Loading existing value into Related model field does not work
Options
Successfully updated the issue's project

There was an error updating the issue's project
Remove unnecessary things from onValueChange callback parameters

Remove unnecessary things from onValueChange callback parameters
Options
Successfully updated the issue's project

There was an error updating the issue's project
use isDirty state for disabling the submit button

use isDirty state for disabling the submit button
Options
Successfully updated the issue's project

There was an error updating the issue's project
implement `nested object` field

implement nested object field
Options
Successfully updated the issue's project

There was an error updating the issue's project
~~implement `dependent field` field~~ (Printing dialog needs to be implemented first)

~~implement dependent field field~~ (Printing dialog needs to be implemented first)
Options
Successfully updated the issue's project

There was an error updating the issue's project
validate that all fields work, especially a choices field

validate that all fields work, especially a choices field
Options
Successfully updated the issue's project

There was an error updating the issue's project
fix TS errors

fix TS errors
Options
Successfully updated the issue's project

There was an error updating the issue's project
Check TODO: comments

Check TODO: comments
Options
Successfully updated the issue's project

There was an error updating the issue's project
Options

netlify · 2023-11-10T10:21:13Z

✅ Deploy Preview for inventree-web-pui-preview ready!

Name	Link
🔨 Latest commit	`8b8ee1c`
🔍 Latest deploy log	https://app.netlify.com/sites/inventree-web-pui-preview/deploys/655b3f263604b90008894d9d
😎 Deploy Preview	https://deploy-preview-5897--inventree-web-pui-preview.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 100 (no change from production) Accessibility: 86 (no change from production) Best Practices: 92 (no change from production) SEO: 70 (no change from production) PWA: - View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

# Conflicts: # src/frontend/src/components/forms/ApiForm.tsx # src/frontend/src/pages/Index/Playground.tsx

wolflu05 · 2023-11-10T10:42:28Z

@SchrodingersGat Please let me know what you think about this implementation, how you feel the performance of the new form on your side compared to latest master. And if we need to improve performance in this PR. In my testing it feels like the latest master has more performance problems than this when running both in dev mode. Trying the production/preview build seems like it is working not really noticeable that there are some lags.

SchrodingersGat · 2023-11-10T12:30:08Z

@wolflu05 nice, I'll check this out next week - I am away for the weekend :)

SchrodingersGat · 2023-11-13T11:51:43Z

@wolflu05 very nice - I like the demo example in the "playground" area :)

wolflu05 · 2023-11-13T13:20:28Z

Thank you @SchrodingersGat that you like it. The reactive approach (where the active switch disables the keyword input) with the inline form without a modal can also be used the same way for the modal form with the hook.

What do you suggest for going forward with this PR? Do you see some performance issues compared to the old implementation? (Make sure to only compare a dev version with a dev version and a build with a build, they have huge differences in my tests.)

SchrodingersGat · 2023-11-13T20:09:07Z

I did not see any performance issues, I have not profiled each implementation.

Do you have a set of features you still want to implement here?

wolflu05 · 2023-11-13T20:56:16Z

No, I have nothing else planned for this PR. Was just not sure if there is some more work required regarding performance. But if you say it's fine, then refactoring the field component to not rerender everytime would be another task.

What is your plan on using the new hook syntax? Should we refactor and remove the old implementation? Or should we just use the new hook implementation for the new forms, or only the ones require reactivity?

SchrodingersGat · 2023-11-14T00:35:37Z

But if you say it's fine, then refactoring the field component to not rerender everytime would be another task.

Would you be willing to take on this one? I think that you have a better idea where to look than I do.

What is your plan on using the new hook syntax? Should we refactor and remove the old implementation? Or should we just use the new hook implementation for the new forms, or only the ones require reactivity?

Can you please submit a PR with an example of refactoring one of the existing forms? I would rather do them all "properly" and refactor what we have now, before we build out a lot more forms.

If you are happy with this, please mark as ready and I will merge

wolflu05 · 2023-11-15T13:40:22Z

Would you be willing to take on this one? I think that you have a better idea where to look than I do.

I tried to eliminate unnecessary dependencies in my last PR, but it seems like the useForm hook from mantine has some issues, see this discussion. The best way to fix this issue and improve performance would be like they recommend in the discussion to switch to another form state library like react-hook-form. I previously already worked with that library.

Can you please submit a PR with an example of refactoring one of the existing forms? I would rather do them all "properly" and refactor what we have now, before we build out a lot more forms.

Sure, I can refactor the old forms in a followup PR if you want.

wolflu05 · 2023-11-15T20:54:20Z

I now committed a first draft of switching to the react-hook-form lib. In my tests the performance now has drastically improved. I already planned the code so that the integration of the nested object and dependent field type become very easy. @SchrodingersGat Please let me know what you think about the move to another form state lib. I attached two screen recordings which shown the performance improvements.

Performance comparison

Old

Bildschirmaufnahme.2023-11-15.um.21.46.48.mov

New

Bildschirmaufnahme.2023-11-15.um.21.47.28.mov

SchrodingersGat · 2023-11-15T22:10:50Z

@wolflu05 looking very nice :)

…orm is not 'dirty'

…idden

wolflu05 · 2023-11-16T18:42:22Z

@SchrodingersGat This would be ready for review now. I decided to implement the dependent field not yet, as 1. the diff is already large enough and 2. it would be better to build the print label dialog first. That would be the easiest think to test the dependent field then while implementing it. But that is a too big task for here.

I now switched to the already mentioned react-hook-form library which drastically improves performance. I already changed a few forms that required a touch because they were incompatible with the new approach of not passing the form object down but rather using the powerful react features. For the rest of the forms I don't have the time now, because I want to finish the machine integration soon.

I also implemented nested object fields now and added the initial_stock and initial_supplier fields to the part create dialog.

matmair

Amazing work; works very well on my laptop and iPad!

src/frontend/src/contexts/LanguageContext.tsx

SchrodingersGat · 2023-11-20T13:00:40Z

After reviewing these changes in #5947 I am quite happy with this implementation - these are great forward steps :)

Please keep this great work coming @wolflu05

wolflu05 · 2023-11-20T13:05:39Z

src/frontend/src/functions/forms.tsx

+  const oldFormSuccess = props.onFormSuccess;
+  props.onFormSuccess = (data) => {
+    oldFormSuccess?.(data);
+    modals.close(modalId);
+  };


@SchrodingersGat To continue this conversation here. This is just a name, because I need to wrap the onFormSuccess function so that I can do something else and also call the callback provided in props. If I wouldn't save this in a variable, and just call it from inside, I would have an endless recursion.

First draft for refactoring the api forms including modals

6d386b9

wolflu05 added 4 commits November 10, 2023 10:26

Merge remote-tracking branch 'upstream/master' into pui-reactive-forms

5a29c85

# Conflicts: # src/frontend/src/components/forms/ApiForm.tsx # src/frontend/src/pages/Index/Playground.tsx

Fix merging errors

d108c75

Fix deepsource

4a038a5

Fix jsdoc

fbaf91d

wolflu05 added refactor Platform UI Related to the React based User Interface labels Nov 10, 2023

trigger: deepsource

7ccdfea

wolflu05 mentioned this pull request Nov 13, 2023

Form improvements #5837

Merged

Try to improve performance by not passing the whole definition down

87e1aef

wolflu05 added 3 commits November 15, 2023 20:36

First draft for switching to react-hook-form

4c9f9ca

Fix warning log in console with i18n when locale is not loaded

048b4d4

Fix: deepsource

81fd9e5

wolflu05 added 6 commits November 16, 2023 09:52

Fixed RelatedModelField initial value loading and disable submit if f…

2cbced7

…orm is not 'dirty'

Make field state hookable to state

89c28b9

Merge remote-tracking branch 'upstream/master' into pui-reactive-forms

1d3d16a

Added nested object field to PUI form framework

3e40d9f

Fix ts errors while integrating the new forms api into a few places

d1a01d1

Fix: deepsource

89f0cbd

Fix some values were not present in the submit data if the field is h…

90496eb

…idden

wolflu05 marked this pull request as ready for review November 16, 2023 18:42

wolflu05 requested a review from SchrodingersGat as a code owner November 16, 2023 18:42

matmair approved these changes Nov 16, 2023

View reviewed changes

src/frontend/src/contexts/LanguageContext.tsx Outdated Show resolved Hide resolved

wolflu05 added 2 commits November 16, 2023 22:34

Handle error while loading locales

6dc619b

Fix: deepsource

53bd75e

wolflu05 mentioned this pull request Nov 17, 2023

[PUI] Api state extension/cleanup #5934

Merged

Merge remote-tracking branch 'upstream/master' into pui-reactive-forms

8b8ee1c

wolflu05 mentioned this pull request Nov 20, 2023

PUI general improvements #5947

Merged

SchrodingersGat merged commit cb53778 into inventree:master Nov 20, 2023
24 checks passed

wolflu05 commented Nov 20, 2023

View reviewed changes

wolflu05 deleted the pui-reactive-forms branch February 15, 2024 14:06

XanderLuciano mentioned this pull request Mar 20, 2024

Make modal footer fixed (#6696) #6699

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make modals/forms more reactive #5897

Make modals/forms more reactive #5897

wolflu05 commented Nov 10, 2023 •

edited

Todo

netlify bot commented Nov 10, 2023 •

edited

wolflu05 commented Nov 10, 2023 •

edited

SchrodingersGat commented Nov 10, 2023

SchrodingersGat commented Nov 13, 2023

wolflu05 commented Nov 13, 2023 •

edited

SchrodingersGat commented Nov 13, 2023

wolflu05 commented Nov 13, 2023

SchrodingersGat commented Nov 14, 2023

wolflu05 commented Nov 15, 2023 •

edited

wolflu05 commented Nov 15, 2023 •

edited

SchrodingersGat commented Nov 15, 2023

wolflu05 commented Nov 16, 2023 •

edited

matmair left a comment

SchrodingersGat commented Nov 20, 2023

wolflu05 Nov 20, 2023 •

edited

Make modals/forms more reactive #5897

Make modals/forms more reactive #5897

Conversation

wolflu05 commented Nov 10, 2023 • edited

Todo

netlify bot commented Nov 10, 2023 • edited

✅ Deploy Preview for inventree-web-pui-preview ready!

wolflu05 commented Nov 10, 2023 • edited

SchrodingersGat commented Nov 10, 2023

SchrodingersGat commented Nov 13, 2023

wolflu05 commented Nov 13, 2023 • edited

SchrodingersGat commented Nov 13, 2023

wolflu05 commented Nov 13, 2023

SchrodingersGat commented Nov 14, 2023

wolflu05 commented Nov 15, 2023 • edited

wolflu05 commented Nov 15, 2023 • edited

Performance comparison

Old

New

SchrodingersGat commented Nov 15, 2023

wolflu05 commented Nov 16, 2023 • edited

matmair left a comment

Choose a reason for hiding this comment

SchrodingersGat commented Nov 20, 2023

wolflu05 Nov 20, 2023 • edited

Choose a reason for hiding this comment

wolflu05 commented Nov 10, 2023 •

edited

netlify bot commented Nov 10, 2023 •

edited

wolflu05 commented Nov 10, 2023 •

edited

wolflu05 commented Nov 13, 2023 •

edited

wolflu05 commented Nov 15, 2023 •

edited

wolflu05 commented Nov 15, 2023 •

edited

wolflu05 commented Nov 16, 2023 •

edited

wolflu05 Nov 20, 2023 •

edited