Skip to content

[WIP] Resize #122

Open
JonForest wants to merge 3 commits intoember-animation:masterfrom
JonForest:resize-on-animated-if
Open

[WIP] Resize #122
JonForest wants to merge 3 commits intoember-animation:masterfrom
JonForest:resize-on-animated-if

Conversation

@JonForest
Copy link

@JonForest JonForest commented Jun 5, 2019

A WIP PR to address #113
The issue here is that when wrapping a resize motion in an animated-if transition, the sprite either starts or ends without initialBounds/finalBounds. This PR allows the caller to provide some initial/final height/width values to act in place of calculated bounds.

I'd really appreciate and feedback on the approach here, as it to my eyes it looks a bit clunky. There might be a cleaner way to do this, so I'm definitely open to feedback.

TODO:

  • Write tests
  • Update docs with options (probably separate PR into gh-pages branch)

).plus(this.prior.widthTween);

this.heightTween = (toHeight === false)
? { done: true }
Copy link
Author

@JonForest JonForest Jun 6, 2019

Choose a reason for hiding this comment

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

I'll fix this indentation issue alongside any other comments.

sprite.finalBounds.height / sprite.finalCumulativeTransform.d, duration
);
this.widthTween = (fromWidth === false || toWidth === false)
? { done: true }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will break when one resize motion interrupts another, because of this line below:

).plus(this.prior.widthTween);

Instead of having a special case here for a missing toWidth or fromWidth, it may be easier to change _extractHeightAndWidthOpts so that if one of those is missing, it substitutes the other. So instead of returning

{
  fromWidth: 100,
  toWidth: false,
  ...
}

it would become:

  fromWidth: 100,
  toWidth: 100, // this automatically defaulted to being the same as fromWidth

This will give you a Tween that doesn't actually move, but that's what we want.

Copy link
Contributor

@ef4 ef4 left a comment

Choose a reason for hiding this comment

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

Thanks, this is looking good other than the comment about the interruption case.

For the tests, a good style of test to copy is

module('Integration | Component | animated container', function(hooks) {
setupRenderingTest(hooks);
setupAnimationTest(hooks);
test("has visual continuity at start", async function(assert) {
this.set('transition', function * () {});
await this.render(hbs`
<AnimatedContainer>
{{#animated-if showThing use=transition duration=1000}}
<div>Content</div>
{{/animated-if}}
</AnimatedContainer>
`);
await animationsSettled();
let before = _bounds(this.element.querySelector('.animated-container'));
time.pause();
this.set('showThing', true);
await settled();
await time.advance(10);
let after = _bounds(this.element.querySelector('.animated-container'));
assert.closeSize(5, after, before);
});

You can use the time controls to interrupt a running motion and cause it to start going back the other way, to make sure that case also works.

…c required for cases where sprite starts or ends by not being present in the DOM
@JonForest
Copy link
Author

Sorry about the massive delay in moving this on.
I've spent some time remember what I was doing, updating the resize motion with your comments and trying to write tests. The one test is hanging at the moment, not sure why.
I'll continue to investigate as time allows.

@denzo
Copy link

denzo commented Apr 10, 2020

Just checking if this is still not possible in ember-animated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants