feat(dialog-renderer): add Animator support close #188

[StrahilKazlachev]

1. Extracted the composition logic from DialogService in a new DialogCompositionEngine - was getting messy.
2. DialogController
   1. Marked the .controller property as internal - can be a breaking change(TS only) if someone used it outside of a Renderer implementation, which I think should not be done.
   2. Added .view internal property.
   3. Introduced new interface InfrastructureDialogController extends DialogController
      • reintroduces the .controller and .view properties - makes them non internal.
      • I really think these properties should only be used by Renderer implementations.
      • a better name is welcomed.
3. Renderer - switched DialogController with InfrastructureDialogController.
4. DialogRenderer
   1. integrated with the Animator
   2. removed the current transition based animation support - breaking change - I don't see how to support both.

TODO:

• [ ] deprecate the ignoreTransitions setting - update docs, leave in interface for now
• [ ] update docs regarding the Animator support
• [ ] update the tests

[EisenbergEffect]

Ultimately, I always wanted this to use the Animator interface. Glad to see this work is progressing!

[StrahilKazlachev]

1. I think that we should let the Renderer decide whether to use a ViewSlot or something else, we should just provide it with the bound Controller/View of the dialog content(the user defined piece).
   • pass a noop ViewSlot to CompositionEngine.prototype.compose - this way the View will not be added to anything prior to providing it to the Renderer. Didn't come up with other solution how to prevent .compose from adding the View to a ViewSlot.
2. For now we should probably let the Renderers decide what of their layout should be animated and how.
   • for the DialogRenderer I've animated the ux-dialog-overlay if ViewSlot.prototype.add/ViewSlot.prototype.remove return a Promise.

[EisenbergEffect]

@PWKad Can you help by reviewing this PR? @StrahilKazlachev Can you provide some more context around the noop ViewSlot bit?

[StrahilKazlachev]

@EisenbergEffect About the noop ViewSlot. The main reason is I only want the CompositionEngine to create/compose a Controller/View, I don't want it to add them(the controller.view or just the view) to a ViewSlot. What I gain:

1. A Renderer can now create the ViewSlot itself, no one else actually needs it.
   • can instantiate it with specific Animator.
2. A Renderer can now first attach the ViewSlot and then call ViewSlot.prototype.add.
   • the View will be attached, then if is marked for animation it will it will be animated - all this will be done by the ViewSlot, a Renderer only needs to properly handle the result of ViewSlot.prototype.add - I'm aware of ViewSlot.prototype.animateView.
   • when Renderer.prototype.hideDialog is called the approach will be symmetrical, just call ViewSlot.prototype.remove and handle the result appropriately.
3. A more sophisticated Renderer could add the Controller/View to its own View hierarchy - still exploring this scenario.

It's not like we can't:

1. create the ViewSlot outside of the Renderer.
2. expose it through the DialogController.
3. after the CompositionEngine.prototype.compose completes call ViewSlot.prototype.remove or every Renderer can do this if it decides to.
4. proceed calling Renderer.prototype.showDialog.

But it but it doesn't feel right to me.

[EisenbergEffect]

Gotcha. Thanks for the explanation.

Sent from my iPhone

On Nov 21, 2017, at 1:34 AM, StrahilKazlachev notifications@github.com wrote:

@EisenbergEffect About the noop ViewSlot. The main reason is I only want the CompositionEngine to create/compose a Controller/View, I don't want it to add them(the controller view or just the view) to a ViewSlot. What I gain:

A Renderer can now create the ViewSlot itself, no one else actually needs it. can instantiate it with specific Animator. A Renderer can now first attach the ViewSlot and then call ViewSlot.prototype.add. the View will be attached, then if is marked for animation it will it will be animated - all this will be done by the ViewSlot, a Renderer only needs to properly handle the result of ViewSlot.prototype.add - I'm aware of ViewSlot.prototype.animateView. when Renderer.prototype.hideDialog is called the approach will be symmetrical, just call ViewSlot.prototype.remove and handle the result appropriately. A more sophisticated Renderer could add the Controller/View to its own View hierarchy - still exploring this scenario. It's not like we can't:

create the ViewSlot outside of the Renderer. expose it through the DialogController. after the CompositionEngine.prototype.compose completes call ViewSlot.prototype.remove or every Renderer can do this if it decides to. proceed calling Renderer.prototype.showDialog. But it but it doesn't feel right to me.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[plwalters]

Awesome work @StrahilKazlachev

Looks like there is still some work in process items, let me know once it is ready and I can pull it down and test further.

[StrahilKazlachev]

As a whole only the tests and docs are WIP, I may refactor some more code, but I'm done with the API changes - depends on feedback.

[EisenbergEffect]

How are we doing on this? Ready to merge?

[jeffgrann]

This issue has been around for a long time. Is this fix going to be in the next release?

[StrahilKazlachev]

@PWKad Quite late, but lets say I'm done for now. One thing I can't really decide on - having 2 DialogController interfaces:

• DialogController - min API surface relevant to app code.
• InfrastructureDialogController extends DialogController - adds additional APIs relevant to Renderer implementations - view: View, controller?: Controller. May be expanded with new features for the renderers.

In my code I would go with having both interfaces, but here I'm not so sure.

[EisenbergEffect]

@StrahilKazlachev I think the two interfaces is fine. Are you ready for final review and merge?

[StrahilKazlachev]

@EisenbergEffect No documentation update in this PR. Was thinking of adding it later. Will try this week. Ignoring the lack of docs update, it is ready for final review.

[EisenbergEffect]

@StrahilKazlachev Can you work on reviewing the docs and seeing if any updates are needed? I can push out a dialog and doc release simultaneously. Just let me know.