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

Expose animator options #55

Merged
merged 1 commit into from
Apr 22, 2018
Merged

Conversation

cammace
Copy link
Contributor

@cammace cammace commented Mar 21, 2018

This extends the Animator class to allow setting durations, interpolators, delays, etc.

ezgif com-gif-maker

Copy link
Contributor

@naturalwarren naturalwarren left a comment

Choose a reason for hiding this comment

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

LGTM.

}

@Override
public Animator setDuration(@IntRange(from = 0) long duration) {
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 the annotations here are useful but we haven't been super consistent with annnotating public APIs. We might want to spin up an issue to track adding them to other places and tag it with good first issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great to hear, I cut a ticket in this repo to track adding more annotations, #62, Unfortunately, as far as merging, I have no power in this repo ;)

Copy link
Contributor

@danh32 danh32 left a comment

Choose a reason for hiding this comment

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

Great idea here! What do you think about making SparkAnimator an abstract class that extends from Animator, so that all future SparkAnimators will also have this functionality? I'm fine merging this as-is as well, just an idea. lmk what you think.

@danh32
Copy link
Contributor

danh32 commented Apr 22, 2018

Actually, that's a backwards incompatible change, unfortunately. I'll have to make it when we make Spark 2.0!

@danh32 danh32 merged commit c18200b into robinhood:master Apr 22, 2018
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.

None yet

3 participants