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

Move source of truth for duration and repeat style to the execution phase #49

Merged

Conversation

NickEntin
Copy link
Collaborator

@NickEntin NickEntin commented Oct 3, 2020

  • Adds duration and repeatStyle parameters to the Animation.perform(...) and AnimationQueue.enqueue(...) methods to allow for specifying an explicit duration and repeat style at the start of the execution phase.
  • Renames the duration properties on Animation and AnimationGroup to implicitDuration.
  • Renames the repeatStyle properties on Animation and AnimationGroup to implicitRepeatStyle.
  • Renames AnimationRepeatStyle.none to .noRepeat to avoid an ambiguous reference when using an optional repeat style.
  • Updates a lot of headerdocs to better explain how the duration and repeat styles are resolved.

Resolves #43.

/// total duration equal to the duration of one cycle (the animation's `duration`) multiplied by the number of
/// cycles (as specified by the animation's `repeatStyle`).
/// total duration equal to the duration of one cycle (the animation's `implicitDuration`) multiplied by the number
/// of cycles (as specified by the animation's `implicitRepeatStyle`).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I want to follow this up with an effort to be more consistent with the terminology we use in the project - both in the long form documentation and headerdocs, as well as some of the (private) variable names. For example, we talk about the "total" duration here, but I don't think that's the most descriptive term. When talking about durations, I've started differentiating by saying:

  • The time from the start of the execution phase to when the animation completes is the end-to-end duration. This is usually a calculated value based on the cycle duration and the repeat style (when available).
  • The time for the animation to go from a relative timestamp of 0 to 1 (or 1 to 0) is the cycle duration. This is typically how consumers specify the duration.
  • The time for the animation to go between two arbitrary relative timestamps is the segment duration. This is used in a few places like snapshot testing and the upcoming interactive animations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes sense, total can be ambiguous when animation has multiple phases. end-to-end sounds good but feels a bit clumsy as variable name, what about overall duration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I could see "overall" working for that. I filed to #52 to track updating the documentation.

@NickEntin NickEntin marked this pull request as ready for review October 5, 2020 10:11
@NickEntin NickEntin added this to the 4.0 milestone Oct 5, 2020
@NickEntin NickEntin force-pushed the entin/duration-and-repeat-in-execution branch from 37c50ae to b995d5d Compare October 13, 2020 08:30
@NickEntin
Copy link
Collaborator Author

Pulled the fixes to the pod lint command into a separate PR (#51)

@NickEntin
Copy link
Collaborator Author

NickEntin commented Oct 13, 2020

Ideas from chatting with @lukebradford last week...

This was commented on the wrong PR 🤦 Moved it to #35 (comment).

…hase

* Adds `duration` and `repeatStyle` parameters to the `Animation.perform(...)` and `AnimationQueue.enqueue(...)` methods to allow for specifying an explicit duration and repeat style at the start of the execution phase.
* Renames the `duration` properties on `Animation` and `AnimationGroup` to `implicitDuration`.
* Renames the `repeatStyle` properties on `Animation` and `AnimationGroup` to `implicitRepeatStyle`.
* Renames `AnimationRepeatStyle.none` to `.noRepeat` to avoid an ambiguous reference when using an optional repeat style.
* Updates a lot of headerdocs to better explain how the duration and repeat styles are resolved.
@NickEntin NickEntin force-pushed the entin/duration-and-repeat-in-execution branch from b995d5d to 537422d Compare October 15, 2020 08:25
Copy link
Collaborator

@seanho seanho left a comment

Choose a reason for hiding this comment

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

Good stuff. The implicit naming adds clarify and easy to reason about.

/// total duration equal to the duration of one cycle (the animation's `duration`) multiplied by the number of
/// cycles (as specified by the animation's `repeatStyle`).
/// total duration equal to the duration of one cycle (the animation's `implicitDuration`) multiplied by the number
/// of cycles (as specified by the animation's `implicitRepeatStyle`).
Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes sense, total can be ambiguous when animation has multiple phases. end-to-end sounds good but feels a bit clumsy as variable name, what about overall duration?

@NickEntin NickEntin merged commit 4fa928d into cashapp:master Oct 26, 2020
@NickEntin NickEntin deleted the entin/duration-and-repeat-in-execution branch October 26, 2020 02:40
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.

Move source of truth for duration and repeat style to the execution phase
2 participants