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

chore(dialog): change animation properties to methods #5464

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vdegenne
Copy link
Contributor

Moving getOpenAnimation and getCloseAnimation properties to methods type for the doc:
image

@asyncLiz
Copy link
Collaborator

Hm, conceptually I think they're still properties in the sense that they're expected to be written, unlike a method which is usually expected not to be changed.

Open to arguments why they should be considered methods though!

@vdegenne
Copy link
Contributor Author

Sorry I don't really have any arguments but for the sake of learning I want to ask why are they considered functions then? why not openAnimation and closeAnimation directly?

@asyncLiz
Copy link
Collaborator

Sorry I don't really have any arguments but for the sake of learning I want to ask why are they considered functions then? why not openAnimation and closeAnimation directly?

There's little technical difference between them. Methods share the same prototype function implementation, while properties are unique per instance.

class Foo {
  propFunction = () => true;

  methodFunction() { return true; }
}

const fooOne = new Foo();
const fooTwo = new Foo();

console.log(fooOne.propFunction === fooTwo.propFunction); // false
console.log(fooOne.methodFunction === fooTwo.methodFunction); // true

Also the methodFunction can also be called on the prototype while the propFunction is harder to call without an instance... but if you're in Foo.prototype.methodFunction.call(...) territory you got bigger problems.

getOpenAnimation and getCloseAnimation are intended to be replaced by the user, while methods on classes are not typically intended to be replaced. So informing tools like lit-analyzer that it's a property can be beneficial.

Technically though, yes, changing it to a shared function instance should theoretically be more performant since we're not creating a new JS function instance per class instance. I doubt there's a measurable impact though.

@vdegenne
Copy link
Contributor Author

Oops sorry @asyncLiz I meant why they should be functions and not an object directly?

class Foo {
  openAnimation = DIALOG_DEFAULT_OPEN_ANIMATION
  // instead of getOpenAnimation() => DIALOG_DEFAULT_OPEN_ANIMATION
}

@asyncLiz
Copy link
Collaborator

Oops sorry @asyncLiz I meant why they should be functions and not an object directly?

I originally made them functions so that users could have more options to create dynamic animations.

dialog.getOpenAnimation = () => {
  if (isM3()) {
    return DIALOG_DEFAULT_OPEN_ANIMATION;
  }

  // A different animation for an app that is
  // iteratively upgrading from M2 to M3
  return M2_MIGRATION_DIALOG_OPEN_ANIMATION;
};

This is just my opinion on what's useful. Would love to hear your dev experience on how you're typically overriding the animations! Lit binding, setting the prop directly, extending, etc.

We could make them both properties and methods if it's convenient devx to bind an object. For example, in a lit binding it's more verbose and creates a function:

html`
  <md-dialog .openAnimation=${OPEN_ANIMATION}>
  <!-- vs -->
  <md-dialog .getOpenAnimation=${() => OPEN_ANIMATION}>
`;

@vdegenne
Copy link
Contributor Author

It just caught my eyes in the docs that's all, I think functions are more handy yes. But in that case we might want to add the keyword await when we call it internally?

@asyncLiz
Copy link
Collaborator

I'm not sure I know a good use case for why animation generation would need to be asynchronous to justify introducing await task timing complexities.

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.

2 participants