feat(LayoutManager): Expose objects registration #9661
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
Build Stats
|
| }[])( | ||
| '$trigger trigger should $action targets and call target hooks', | ||
| ({ action }) => { | ||
| const lifecycle: jest.SpyInstance[] = []; |
There was a problem hiding this comment.
this 3 tests were failing because i wrote bad code. And it was not immediate to me to understand what they were testing for 2 reasons:
The connection between the array of data and the test variables force me to look up and down every time to understandand what is action and what is trigger, and the lifecycle array was a convoluted way to spell 3 times what has been calle, but without clearly saying with what has been called.
In general we should keep those array method for tests either for simple cases [true, false] or for large cases in which we need to use points or something else.
In the other cases i really prefer to spell clearly what the test is doing
| const { target } = context; | ||
| this.unsubscribe(object, context); | ||
| const disposers = [ | ||
| object.on('modified', (e) => | ||
| this.performLayout({ | ||
| trigger: 'modified', | ||
| e: { ...e, target: object }, | ||
| e, |
There was a problem hiding this comment.
in the object modified, target is already available in the event, we don't need to repeat it. At leat this says typescript
There was a problem hiding this comment.
on line 88 ( now 93 ) we do the same for the other event, but in my opinion we shouldn't. If we want to forward the original event that is fine, but we are adding the target itself in the object event, we don't need to copy it in the forwarded event.
There was a problem hiding this comment.
I remember something about this.
If you wish to do this you should add tests to verify this.
Notice that we add a target prop to this object.
There was a problem hiding this comment.
I see that there is a target prop to this object, that is why i don't see any reason to duplicate it on the event.
Probably also e should be called 'triggerEventData' or something that can let us know what 'e' is because is not clear. The only purpose of this 'e' is to be found later inside 'layout:after' event, correct? What else?
There was a problem hiding this comment.
What I don't like about this is that is makes the API missing
Who knows what context the dev needs to decide if to subscribe/unsubscribe targets?
I do not want to reduce the available context.
I suffer from it when I work with fabric and then need to do complex overriding to provide a missing bit of context.
|
We can add more context, but the logic should be, we write the minimal code, and we extend if a use case will surface. also stopPropagation should have the same treatment. I don't see where are we using it. Either provide an example in some doc/readme of why is there or we need to remove it. |
…ricjs/fabric.js into separate-registration-layout-manager
|
I restored the full context, missing there was just the |
Simple case |
|
We are discussing a single flag. IMO it is too much time and resources spent on it. |
If you don't have time you don't have time. I'll see what i can do to finish the LayoutManager |
asturur
changed the title
asturur
changed the title
Description
When we stopped running the layout manager from group that comes from a json object, we removed also the object registration part and that broke interactive groups that won't react when the objects are transformed.
This PR fixes the issue by explicitly registering the objects.
This is a temporary fix, the final goal will probably be to deprecate the interactivity property of the group and add a setInteractive method that will also take care of event registration, since it doesn't seems right to have event handler registered anyway for objects that are non interactive.
There are probably other parts in the transform lifecycle where we can handle this but for now we are registering everything every time.