Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toggle animation and layoutEnd event #11

Closed
wants to merge 11 commits into from

Conversation

jitsmaster
Copy link
Contributor

Added css based animation when split area toggle visibility. The animation is off by default

add layoutEnd event on transitionend during split area visibility toggle.

jitsmaster and others added 7 commits January 24, 2017 16:45
of area, instead of conditional create the area.

Also, extracted some hard-coded styles into css classes, that will be
contained within the component.
transitionend events of the split areas, after show/hide animations
finishes.

Also, upgrade Angular 2 to 2.4.3.
are supported in native view encapsulation
@@ -201,6 +207,21 @@ export class SplitComponent implements OnChanges, OnDestroy {
this.areas.sort((a, b) => +a.orderUser - +b.orderUser);
}

if (this.areas.length > 1) {
var l = this.areas.length;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused var..

@@ -201,6 +207,21 @@ export class SplitComponent implements OnChanges, OnDestroy {
this.areas.sort((a, b) => +a.orderUser - +b.orderUser);
}

if (this.areas.length > 1) {
var l = this.areas.length;
var c = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused var..

if (this.areas.length > 1) {
var l = this.areas.length;
var c = 0;
var sub = this.areas[0].component.sizingEnd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird stuff that could lead to increasing number of subscriptions (refresh is called a lot of times).
'debounceTime' of 500ms make the EventEmitter emits at wrong times I imagine?
I will try to find a way to avoid a subscribtion to child eventEmitters... For me, splitArea components should notify split component directly calling a function on it because they already have it injected.

@bertrandg
Copy link
Contributor

I didn't play with it already but I've seen several things to change before merging it.

@bertrandg
Copy link
Contributor

I'm afraid layoutEnd event name could lead to confusion between use of *ngIf and [visible].
visibleTransitionEnd should be clearer about the target.

And I will make a warning about using [visible] instead of *ngIf, that's a good thing to have the option for specific cases but it could lead to performance degradation due to all DOM/bindings/components still calculated even if not visible.

@jitsmaster
Copy link
Contributor Author

Thank you very much for looking into it. Unfortunately I can't do anything yet. Snow storm hit Seattle area. My house is out of power. Estimated to be fixed by tomorrow. Then I can get back on it.

You did some great refactors to my last pull request. Appreciate it.

@bertrandg
Copy link
Contributor

No problem! :)
Take care of you and your home.

@jitsmaster
Copy link
Contributor Author

Finally have time to get back on it. "visibleTransitionEnd" is a better name. I am thinking about the possibility of having layoutEnd to include ngIf scenarios too. But that is a much bigger refactor, means to hook up observables in tabs toggling, so the ngIf can use async pipes, and split component and capture those events and use layoutEnd.

We will just call the event visibleTransitionEnd for now.

I will put it the simple fixes and look into simple area event for visibility toggle

@bertrandg
Copy link
Contributor

Cool, take a look at this plunker, I found a way to avoid any subscription in split.component:
https://plnkr.co/edit/epjWLs?p=preview
Let me know if you want to add it or I will do it myself during the merge.

Yes having transition for ngIf too would be awesome but I have really no idea how it could works..

@jitsmaster
Copy link
Contributor Author

Are you talking about using Subject and asObservable?

The subject still needs to be triggered somehow.

I am putting together the changes now that will reduce the amount of subscriptions, and clean them up up front.

@jitsmaster
Copy link
Contributor Author

I normally use BehaviorSubject, because it can be triggered repeatedly. Subject is normally for fire and forget

Ditched the centralized merged subscriptions and hooked up to invididual area sizingEnd event.
Prevent multiple firing of sizingEnd event from split areas, by limiting to flex-basis only on the event property.
Update comments on visibleTransitionEnd to better describing how to subscribe to it.
Use BehaviorSubject instead of eventemitter to prevent outside party triggering this event.
@jitsmaster
Copy link
Contributor Author

Done.
I figured out what is causing the repeated transitionend hits. So merging is no longer that needed anymore.
Each are gets one subscription in its life time only.

I kept the structure of subject.asObservable. It is not very necessary, just prevent outside party from triggering the event. You can change it back to EventEmitter if you feel like it.

@bertrandg
Copy link
Contributor

My plunker was about the fact I avoided any subscription inside split.component.ts (no .subscribe() .unsubscribe()). The only subscription occurs if, and only if, user request it from outside in his template:
<split (visibleTransitionEnd)="log($event)">

I used Subject in the plunker because angular EventEmitter extends it.
From what I've just learned, BehaviorSubject differ from it just by sending its last value when we subscribe to it, not needed here it seems...
http://stackoverflow.com/questions/39494058/angular-2-behavior-subject-vs-observable

I will try to merge it next weekend.

@jitsmaster
Copy link
Contributor Author

Thank you

@jitsmaster
Copy link
Contributor Author

I see what you mean.
We could have the area directly call the subject in the split component. I always avoided that practice, because that will tight-couple the split component and its areas. I guess they are already tightly coupled, so it is no big deal.

I will make that change.

remove the sizingEnd event from areas, and have them directly invoke notify function on split component.

Area was already tightly coupled with split component, so this is a better way to make thing happen.
@jitsmaster
Copy link
Contributor Author

jitsmaster commented Feb 14, 2017

The last changed removed the event driven system altogether for for visibleTransitionEnd event gets triggered.

@bertrandg
Copy link
Contributor

Yes you got it,
It simplify the code and, like you said, they are already tight-coupled because split is injected in splitArea.

Now, if there isn't need for debounce/distinctUntilChange/merge.. operators anymore, we can get rid of _visibleTransitionEndSub, transform visibleTransitionEnd to:
@Output() visibleTransitionEnd = new EventEmitter<Array<number>>(false);
and directly call it from notify().

I was asking myself, just by curiosity, what is your use case which need visibleTransitionEnd from outside? What do you do when handling it?

thanks ;)

@jitsmaster
Copy link
Contributor Author

The use case is same as dragEnd. I am doing some crazy Angular 2 implementation by use angular component as shell to wrap up dojo widgets. Those dojo widgets are manually layed out and depending on the event to resize itself. I have left panel in the split container that toggle on/off all the time, and right side content widgets need to listen to the toggle event and resize itself.

@jitsmaster
Copy link
Contributor Author

Have you had he chance to merge it in last weekend?

Sorry for bug you, but AOT compiler doesn't like npm links

@jitsmaster
Copy link
Contributor Author

Actually, no worry. I figured out how to to npm install directly from github repo. Just some minor twick on .gitignore to make things work.

Glad you are including everything in dist folder. All I have to do is removing the src in dist folder to fix AOT compiler issue.

I am good for now.

@bertrandg
Copy link
Contributor

I'm on it, I merged it and pushed it in 'test-pr' branch:
https://github.com/bertrandg/angular-split/tree/test-pr

But when testing, I've seen that visibleTransitionEnd event is sent X times (once for each areas) for one toggling..
https://plnkr.co/edit/GyQnxE?p=preview
Check it and tell me if it was same behavior on your version or if it comes from the way I merged it.

@jitsmaster
Copy link
Contributor Author

That was the reason I went with the more complex approach to debounce the event and make it only fire once. It is indeed what I see.
It doesn't cause too much problem for actual implementations, since the subscription part can always debounce it .

Do you want me to revert it back to the old way?

@bertrandg
Copy link
Contributor

Ok, no thanks I will add it.

@bertrandg
Copy link
Contributor

Merged and published 0.1.20 :)

@bertrandg bertrandg closed this Mar 11, 2017
@jitsmaster
Copy link
Contributor Author

jitsmaster commented Mar 11, 2017 via email

@bertrandg
Copy link
Contributor

Thanks to participate there.
I'm happy to see people using and improving angular-split!

@angular-split angular-split locked and limited conversation to collaborators Feb 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants