PolymerElements / iron-dropdown

An unstyled element that works similarly to a native browser select
https://www.webcomponents.org/element/PolymerElements/iron-dropdown
34 stars 50 forks source link

Remove deprecated neon animation, use CSS keyframe animations #154

Open valdrinkoshi opened 6 years ago

valdrinkoshi commented 6 years ago

Fixes #147 by removing neon-animation dependency in favor of CSS keyframe animations, taking a similar approach as in https://github.com/PolymerElements/paper-dialog/pull/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 https://github.com/PolymerElements/paper-menu-button/pull/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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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.