openlawteam / openlaw-elements

Render React form components and sections from an OpenLaw template.
https://docs.openlaw.io/openlaw-elements/
Apache License 2.0
15 stars 11 forks source link

Pure render for "variable" components #46

Closed jtrein closed 5 years ago

jtrein commented 5 years ago

This is the base part of work to get OpenLawForm on large agreements to be performant. In my testing this does alleviate some performance issues with the large agreements (e.g. LSTA)! There is more work to be done in our app, specifically, but that's not really the business of this PR.

Work done

@jdville03 Can you give this a test-run in the openlaw web app, or the example app, to make sure I haven't damaged anything? In addition to my own testing, I've run npm test and sbt test (the sbt tests that are failing for sbt aren't b/c of this) on the openlaw app side with these components.

Note: If you're testing in openlaw web app, after using yarn link, you'll probably have to uncomment flow before building, otherwise there will be some incorrect flow errors.

jdville03 commented 5 years ago

Sure I'll test this out today.

jdville03 commented 5 years ago

So far I've found regression with the Identity Variable (testing in openlaw web app). The field clears after typing the @ in an email. I left the devtools open in the video below in case helpful to see the component props and state.

https://www.useloom.com/share/23ac85d96b81411cab39b6333134e7d1

jdville03 commented 5 years ago

There is also regression with the YesNo variables.

This first set of videos shows the expected behavior and regression when they're used in a deal. Expected behavior (the conditional question remains in the deal agreement form when it's clicked on): https://www.useloom.com/share/9d8531885f384e6ab2f71e8a6235223b Regression (the conditional question gets removed from the deal agreement form when it's clicked on): https://www.useloom.com/share/03c071adfed2450e9c25894a69ab8ad8

This second set of videos shows the difference in the context of a flow form. Expected behavior (the radio is rendered correctly as checked when clicking on it, leaving the form page, and coming back to that form page): https://www.useloom.com/share/059b9e1f0086471198e7f42672286dce Regression (the radio button is no longer checked when clicking on it, leaving the form page, and coming back to that form page): https://www.useloom.com/share/f3186f112f7644159513d75e6d478d08

jdville03 commented 5 years ago

These tests are new failures for me in the openlaw web app when I'm running with the test local package in this pr:

org.adridadou.openlaw.demo.EmployeeOnboardingE2ESpec
org.adridadou.openlaw.e2e.StopContractE2ESpec
org.adridadou.openlaw.e2e.EthereumCallE2ESpec

They all rely on data being inputted into Identity variables and/or YesNo variables being clicked on. The failures might be related to the regression I found with those variable types.

There are a couple of other items I'll need to check in the openlaw web app, but those are dependent on the Identity and YesNo variables working correctly.

jdville03 commented 5 years ago

This video shows regression with the Period type. It's popping an error after the number is typed.

https://www.useloom.com/share/9f136e4bc8494b2aa802318eb7a6b966

jtrein commented 5 years ago

This is great stuff, I'm going to dig into this and try to reproduce and fix. Thank you.

jtrein commented 5 years ago

@jdville03 OK, I narrowed it down to the way I was handling the cached function for Openlaw.checkValidity. Fixes are pushed.

jtrein commented 5 years ago

@jdville03 OK, className has been replaced for Date that was causing tests to fail in openlaw.

jdville03 commented 5 years ago

Still seeing the issues with the YesNo variable after the new fixes: https://github.com/openlawteam/openlaw-elements/pull/46#issuecomment-474597057

jtrein commented 5 years ago

@jdville03 OK, YesNo fixes have been pushed!

jtrein commented 5 years ago

Thank you! Maybe separately, just in case?

jtrein commented 5 years ago

Actually, we don't have any package dependency on openlaw, as it's a prop. We could mention something in the README about what versions we support, which I think right now is anything published in the last few months (guessing here).