WIP: Implement multiple <AnimatedOrphans/> support by nickschot · Pull Request #174 · ember-animation/ember-animated

nickschot · 2019-10-21T15:18:10Z

This implements #169

In the current implementation it is necessary for every <AnimatedOrphans/> to be a block component as animators will only check against ancestor <AnimatedOrphans/>.

The "original" component is now situated in {{-animated-orphans}} and a new top level <AnimatedOrphans/> component has been added (with minimal functionality of its own). Trying to make the original component into a block one resulted in at least some margin collapse issues.

In the current implementation the {{-animated-orphans}} component will check if it is the closest ancestor of the removed component. It might be better to do this in the matchDestroyed function in the motion service.

nickschot · 2019-10-21T15:18:56Z

@ef4 please let me know if this is what you had in mind or if changes are necessary. I'll update docs & tests if the concept is approved!

As the entire component was moved I'll also place some pointers of what has changed in the (old) animated-orphans component (now -animated-orphans).

nickschot · 2019-10-21T15:20:37Z

addon/components/-animated-orphans.js

+    if(!this.parent) {
+      throw new Error(`{{-animated-orphans}} cannot be used directly, use {{animated-orphans}} instead.`);
+    }


Check if a parent was passed (no validity check (yet?)).

nickschot · 2019-10-21T15:22:11Z

addon/components/-animated-orphans.js

+    const _removedSprites = removedSprites.filter(s => {
+      let closestAnimatedOrphans;
+
+      // find closest ancestor <AnimatedOrphans/> that is not in the process of being destroyed
+      for(let ancestorComponent of ancestorsOf(animatorComponent)) {
+        if (ancestorComponent.isEmberAnimatedOrphans && !ancestorComponent._isDestroying) {
+          closestAnimatedOrphans = ancestorComponent;
+          break;
+        }
+      }
+
+      return closestAnimatedOrphans === this.parent;
+    });


Finds the removedSprites of which this AnimatedOrphans is the closest ancestor

nickschot · 2019-10-21T15:30:54Z

addon/components/-animated-orphans.js

+    if(_removedSprites.length){
+      this._newOrphanTransitions.push({
+        removedSprites: _removedSprites.map(sprite => {
+          // we clone the owner objects so that our sprite garbage
+          // collection is entirely detached from the original
+          // animator's
+          sprite.owner = sprite.owner.clone();
+          sprite.owner.flagForRemoval();
+          return sprite;
+        }),
+        transition,
+        duration,
+        shouldAnimateRemoved
+      });
+      this.reanimate();
+    }


Only reanimate if any removedSprites were found for this AnimatedOrphans

nickschot · 2019-10-21T15:31:35Z

addon/components/-animated-orphans.js

+function * ancestorsOf(component) {
+  let pointer = component.parentView;
+  while (pointer) {
+    yield pointer;
+    pointer = pointer.parentView;
+  }
+}


This is copied over from the motion service, might want to extract or export & reuse.

ef4 · 2019-10-23T15:34:47Z

Thanks, this is looking good. I think you're right to split this into two components. The inner one wants to be able to manually append DOM nodes to its element, which means we don't really want any other glimmer-managed content in there.

In the current implementation the {{-animated-orphans}} component will check if it is the closest ancestor of the removed component. It might be better to do this in the matchDestroyed function in the motion service.

I agree, I think that matching can happen in the motion service.

This is copied over from the motion service, might want to extract or export & reuse.

Yes, and it's also possibly using private API, so it probably belongs in https://github.com/ember-animation/ember-animated/blob/98b32fa004ce13390429bd55679ebb7838102f89/addon/-private/ember-internals.js

ef4 · 2019-10-23T15:37:18Z

One other API-design issue: since <AnimatedOrphans/> is not totally DOM-less and since it will now be wrapping people's own layout, an important goal is that

It only produces one layer of <div> ( this is already true in your implementation, but let's keep it that way).
The user can control everything about that <div> (including the tag and html attributes). We already do this for <AnimatedContainer/>, so this can follow the same pattern.

nickschot · 2019-10-23T17:23:51Z

One other API-design issue: since <AnimatedOrphans/> is not totally DOM-less and since it will now be wrapping people's own layout, an important goal is that

It only produces one layer of <div> ( this is already true in your implementation, but let's keep it that way).

The user can control everything about that <div> (including the tag and html attributes). We already do this for <AnimatedContainer/>, so this can follow the same pattern.

Which div do you mean? The top-level component has no tag. There is only the div.animated-orphans which does not wrap anything except for the orphans.

ef4 · 2019-10-23T17:54:19Z

Oh yes, you're right. I was thinking of div.animated-orphans, but you're right, it doesn't end up wrapping. Good, that's even better. 👍

ef4 · 2019-10-23T18:19:08Z

I wonder if <AnimatedOrphans/> is still the right name for this component now that it will be used in block form. Maybe we can brainstorm some alternatives that make it clear why you're wrapping a particular section of the app in this block.

The original name worked because you were just making a place for animated orphans to render. But now you're also expressing something more.

nickschot · 2019-10-23T18:21:38Z

In the current implementation the {{-animated-orphans}} component will check if it is the closest ancestor of the removed component. It might be better to do this in the matchDestroyed function in the motion service.

I agree, I think that matching can happen in the motion service.

For the matching to happen there we do also need to keep track of the AnimatedOrphans instances (currently its just the callbacks).

nickschot · 2019-10-23T18:57:59Z

Also, currently there is no way to do this (which happens frequently in tests):

<div style="position: fixed; top: 0; left: 0">{{animated-orphans}}</div>

Should we provide a way to pass on styles to -animated-orphans? Splattributes are going to be confusing here because the parent to which you'd pass the attribute is wrapping all content.

ef4 · 2019-10-23T20:18:41Z

Ah, so yeah, this is kinda what I meant about letting people set their own tag and attributes. And I can see how it's confusing.

Maybe we should just make the outer component have the Element, and then it does wrap the user's content, and then it behaves more like how it looks in the template. Angle bracket invocations that don't actually generate any DOM (or in this case, that generate DOM with a different structure than they look like) feel a bit weird to me. For that reason I've tried to favor curlies for DOM-less things like {{#animated-if}} (because it's like {{#if}} and only show angle bracket for things that generate DOM (like <AnimatedContainer />).

kturney · 2020-01-06T17:14:57Z

Hi all.

Are there any new thoughts/happenings around this feature?
I've found myself needing the same feature described in #169.

nickschot · 2020-01-07T10:14:33Z

@kturney I unfortunately did not have the time to finalize this PR yet. I will take a look at updating the branch to master later today.

…rphans # Conflicts: # addon/-private/ember-internals.ts # addon/components/animated-each.ts # addon/components/animated-orphans.ts # addon/services/motion.ts

nickschot · 2020-01-07T19:20:53Z

I've updated this branch to work with master again (& TS), not made any other changes yet.

…rphans

nickschot · 2020-06-04T12:01:02Z

Ah, so yeah, this is kinda what I meant about letting people set their own tag and attributes. And I can see how it's confusing.

Maybe we should just make the outer component have the Element, and then it does wrap the user's content, and then it behaves more like how it looks in the template. Angle bracket invocations that don't actually generate any DOM (or in this case, that generate DOM with a different structure than they look like) feel a bit weird to me. For that reason I've tried to favor curlies for DOM-less things like {{#animated-if}} (because it's like {{#if}} and only show angle bracket for things that generate DOM (like <AnimatedContainer />).

I'm working on an implementation for this now. We can brainstorm a new name once we're happy with implementation.

nickschot · 2020-06-04T12:54:03Z

So working on this I ran into a couple of things:

Moving everything into a single wrapping causes some problems/overhead because sprites are matched based on all direct descendants of the element. It is possible, but probably not desirable.
Taking the above into account the private {{-animated-orphans}} would still also require an element/tag. It can be positioned absolutely relative to the public <AnimatedOrphans/> though.
Margin collapse causes problems. I modified a demo where the sprites have a margin which (when not animated) breaks out of the <AnimatedOrphans/> element making everything pop up on animation. While the animation is running and when you keep clicking this the element keeps moving up.

@ef4 any thoughts/ideas?

Initial work on implementing multiple AnimatedOrphans support

ecafa79

nickschot commented Oct 21, 2019

View reviewed changes

remove console.log

5689cbb

nickschot commented Oct 21, 2019

View reviewed changes

fix lint error

80e7179

move ancestorsOf function to -private/ember-internals

d0d3d8e

find closestAncestor in motion service instead of -animated-orphans

c4d814f

nickschot added 5 commits January 7, 2020 17:35

Merge remote-tracking branch 'origin/master' into multiple-animated-o…

497aa52

…rphans # Conflicts: # addon/-private/ember-internals.ts # addon/components/animated-each.ts # addon/components/animated-orphans.ts # addon/services/motion.ts

fix typescript upgrade related issues

a8ba1ad

run prettier

a6f184b

fix PascalCasing issue

00472b3

add -animated-orphans component to lint exclusions

2d3e754

Merge remote-tracking branch 'origin/master' into multiple-animated-o…

56f26b0

…rphans

Conversation

nickschot commented Oct 21, 2019

Uh oh!

nickschot commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickschot Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

nickschot Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

nickschot Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

nickschot Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

ef4 commented Oct 23, 2019

Uh oh!

ef4 commented Oct 23, 2019

Uh oh!

nickschot commented Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ef4 commented Oct 23, 2019

Uh oh!

ef4 commented Oct 23, 2019

Uh oh!

nickschot commented Oct 23, 2019

Uh oh!

nickschot commented Oct 23, 2019

Uh oh!

ef4 commented Oct 23, 2019

Uh oh!

kturney commented Jan 6, 2020

Uh oh!

nickschot commented Jan 7, 2020

Uh oh!

nickschot commented Jan 7, 2020

Uh oh!

nickschot commented Jun 4, 2020

Uh oh!

nickschot commented Jun 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nickschot commented Oct 21, 2019 •

edited

Loading

nickschot commented Oct 23, 2019 •

edited

Loading