Skip to content
This repository has been archived by the owner on Dec 19, 2024. It is now read-only.

Remove deprecated neon animation, use CSS keyframe animations #154

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

valdrinkoshi
Copy link
Member

@valdrinkoshi valdrinkoshi commented Mar 30, 2018

Fixes #147 by removing neon-animation dependency in favor of CSS keyframe animations, taking a similar approach as in PolymerElements/paper-dialog#163.

<iron-dropdown> now ships 4 animations: fade-in-animation, fade-out-animation, scale-up-animation, scale-down-animation, and supports custom animations too.

We keep the openAnimationConfig, closeAnimationConfig, animationConfig properties, and warn that they won't be affecting the animation.

We keep all the public methods previously inherited from neon-animation-runner-behavior, and make them no-op.

After testing this together with PolymerElements/paper-menu-button#137 on internal projects, it doesn't break them - as most of them rely on the default animations setup by paper-menu-button.

Here a test w/ paper-menu-button http://jsbin.com/mafegan/1/edit?html,output

@homerjonathan
Copy link

homerjonathan commented Mar 30, 2018

What about the expanding animation. This was quite important for the Material Design.

It uses the updateStyles to push the height into the CSS so that it can get past the Animation restriction of having a fixed size for the from and to.

@valdrinkoshi
Copy link
Member Author

I did a quick example in the demo page here by animating max-height, and did the proper animation in paper-menu-button using the transform + inverse transform technique

@valdrinkoshi
Copy link
Member Author

Here you can test this implementation http://jsbin.com/mafegan/1/edit?html,output
You can compare with master by uncommenting the lines that import master (see jsbin)

@homerjonathan
Copy link

I found in my testing this code useful:

 if (style.getPropertyValue('animation-name') !== 'none' &&
            style.getPropertyValue('animation-duration') !== '0ms') {
            // How many Animations are legal?  Count the commas!
            for (var count = -1, index = -2; index != -1; count++, index = style.animationName.indexOf(",",
                index + 1));
            this.noValidAnimations = count;
            return true;
          } else {
            this.noValidAnimations = 0;
            return false;
          }

What I found if someone selected the wrong animation or the animation was invalid the animation end event did not fire. This caused the component to be fixed in a state and would not close. It also meant that if the user added more than one animation it would wait until they are all completed before changing state.

Then a simple counter to know when all the animations have finished.

if (this.noValidAnimations !== 0) {
            // Take One Away until we reach zero - return not complete yet
            this.noValidAnimations--;
            return;
          }

May be useful.

@homerjonathan
Copy link

Did you get a good look at my expand-animation in my PR?

      #contentWrapper.animating ::slotted(*) {
        overflow: hidden;
        pointer-events: none;
        max-height: inherit !important;
        height: inherit;
      }
      .expand-animation {
        animation-name: expand-animation;
      }

      @keyframes expand-animation {
        from {
          max-height: calc(var(--animation-height) / 2);
        }
        to {
          max-height: var(--animation-height);
        }
      }

So the trick was to send in the height.

          this.updateStyles({
            '--animation-height': this.containedElement.style.maxHeight
          });

This I assume would work for expanding width as well. From the number of requests for help in animation and heights this may be a useful technique. I don't think it is quite as cool as your inverse technique, but mine is more simple to take advantage of.

@valdrinkoshi
Copy link
Member Author

valdrinkoshi commented Mar 30, 2018

@homerjonathan regarding the multiple animations, that should be doable, but I'd rather do that in a separate PR - I'm convinced that supporting a single animation should be enough for most of the purposes.

Regarding your PR, yes I did review it. That technique assumes that you'd always want to animate max-width/max-height for half the size to full size, which might not be true. Also, it seems to require the user to set max-height on the .dropdown-content element, right?

<iron-dropdown id="dropdown"
               open-animation="expand-animation"
               close-animation="fade-out-animation">

    <paper-listbox slot="dropdown-content"
                   style="max-height: 180px">

        <paper-item>Share</paper-item>
        <paper-item>Settings</paper-item>
        <paper-item>Help</paper-item>

    </paper-listbox>

</iron-dropdown>

I tested this and cannot get it working http://jsbin.com/vatama/4/edit?html,output

In any case, I don't think <iron-dropdown> should ship expand-animation as it is a custom animation, not something shipped by neon-animation. As such, it should be implemented by the user of iron-dropdown, e.g.

@keyframes expand-animation {
  from { max-height: 90px; opacity: 0; }
  to   { max-height: 180px; }
}
.expand-animation {
  animation-name: expand-animation;
}

@homerjonathan
Copy link

homerjonathan commented Mar 30, 2018

Your right my code has stopped working. Not sure what has changed. It seems to grow, shrink to the 50% and then grow. Not quite what was intended ;-)

The reason I was trying to keep expand-animation working is that its part of paper-menu-button it uses paper-menu-grow-height-animation. That is where the 50% height comes from. So that would break the component.

The general idea was to provide the paper-menu-grow-width-animation and paper-menu-grow-height-animation. With the intention of moving users of the component to CSS as the better way.

As for the max-height there is no settings. In the demo given the menu has not max-height. The browser calculates the height and throws it in. So to animate the menu in the Material Designed way (or at least how the paper-menu-button does it) is that 50% to 100% height only growth.

@valdrinkoshi
Copy link
Member Author

Yep, I agree we should provide an alternative to paper-menu-grow-height-animation, but that should be done by paper-menu-button, and not iron-dropdown.

Since we are removing neon-animation from iron-dropdown, we should try to provide the most common animations neon-animation provides, e.g. fade in/out and scale up/down.

*/
openAnimationConfig: {
Copy link
Member Author

Choose a reason for hiding this comment

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

this might break bindings done via html. We should probably keep these and warn if they get set, e.g.

ready: function() {

  // ...
  if (this.openAnimationConfig || this.closeAnimationConfig || this.animationConfig) {
    console.warn('Configuring animations through animationConfig, ' +
                 'openAnimationConfig, or closeAnimationConfig is no longer ' +
                 'supported. Use entryAnimation and exitAnimation instead.');
    // Set this for backwards-compat.
    this.animationConfig = this.animationConfig || {
      open: this.openAnimationConfig,
      close: this.closeAnimationConfig
    };
  }
}

And update the description to something like

/**
 * Deprecated, use `entryAnimation` instead.
 * `iron-dropdown` doesn't depend anymore on `neon-animation`, and this property is kept
 * here to not break bindings. Setting it won't have effects on the animation.
 * @deprecated
 */

@e111077 e111077 mentioned this pull request Jan 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Polymer 3.0 support: Remove neon-animation dependency
3 participants