New conditions handler (defined globally for schema instead of locally for field) #652
Conversation
Hyperkid123
added
enhancement
feat: better example conditions in the schema feat: debug-div changed to pre for better JSON-rendering chore: dispatchCondition renamed to dispatchUIState for clarity chore: dispatch after parseCondition is now separated into two different dispatches (add/remove) for clarity
|
So the rendering performance seems solid. Looking at the profiler, the only time when multiple fields are re-rendered in the same cycle is when a condition is met/broken which makes sense and has to happen. 👍 on that. I will now dig a bit deeper trough the code. |
There was a problem hiding this comment.
Overall this looks very good and would a great addition. Other than just a convention difference from current code (which can be addressed later) I have just a small number of comments.
But the approach is good and I like it. Once you are done with your checklist we will make a thorough review. 👍
chore: switched from for-loop to entries-iteration chore: moved utility functions isObject and isArray to outside of the conditionsMapper
|
DONE:
|
N.B. legacy condition definitions will not work in this version. A soon to come legacyConditionsMapper will solve this. chore: replaced condition.js with condition2.js (new handler) chore: removed Conditions and FormConditionsWrapper components from render-form.js chore: implemented new conditions-handler in render-form.js, including support for "visible" and "disabled"
|
DONE (98d2064):
|
|
@Hyperkid123 Some design questions: Do you want the conditions collection to be an object or an array? Object pros:
Array pros:
Is support for sequence still needed? You said earlier that you wanted to get rid of sequence conditions. Does this mean that the legacyConditionsMapper does not have to cover sequences? |
* master: Release of new version: 2.8.0 <no> [skip ci] Add resolveProps type PF3 datepicker test timezone fix Add documentation page for resolve props Add tests for resolve props feat(renderer): add resolveProps option # Conflicts: # packages/react-form-renderer/src/form-renderer/render-form.js - manually merged
|
@Hyperkid123 I can't find any repo for the documentation. Thought I'd update the docs for the new conditions schema. |
…s otherwise fix: field.hideField was not taken into consideration in FormFieldWrapper
…conditions for now
msageryd
changed the title
|
@Hyperkid123 I've implemented a naive legacyConditionsMapper. It can only handle simple visibility conditions for now. This needs to be revisited in order to handle more complex conditions. f1818eb |
|
@Hyperkid123 I have started to update the tests for conditions. How do I run the tests? |
|
@msageryd sorry for the silence (weekend). Going through the changes now. |
This might cause issues with the value initialization and submission. There is a difference between the condition wrapper and the hidden wrapper. If the condition is not met the field is not in the DOM and once the condition is met you can trigger mounting events like re-initialize the field. So if the condition is false the value is not sent to the submit callback. Yes value of the condition can be persisted even if it's hidden but that is aa opt-in attribute. If the hideField is So the condition wrapper might no longer be required but we still need to detach/attach the field from/to the DOM. |
From the development point of view, the object is preferable. But the order of execution is important and it makes it a bit easier for a user to understand and define the condition order. That is ultimately the reason why the whole schema is a nested array since its sometimes created by users who do not necessarily have any experience with any development. I would like to stick with the Array if possible.
Sequences still have to be supported. We have to keep them at least until the next major release. We introduced them to ensure the correct order of execution of conditions using AND and OR statements when we introduced the |
its the I would like to add that if run locally, the docs use locally build packages so it is required that all packages are built on your local machine. Running |
There is the same requirement as with the docs. All packages must be built locally in since the tests use a local build of all packages. This is there to ensure that no matter what changes in what packages you make, it will run all the tests with all of these changes. If you have all packages build already and you still see some error, just post the stack trace here and we can look at it. But looking at the circle CI log, you have a circular dependency so the renderer package wont even build: That may be the reason why the tests fail. |
Hyperkid123
force-pushed
the
master
branch
3 times, most recently
from
59a2001
to
4bf9530
Compare
My proposal for a new conditions handler:
TODO:
I'm sure there are lots to discuss before this reaches production.