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

CSS animations #810

Merged
merged 1 commit into from
Jul 5, 2021
Merged

CSS animations #810

merged 1 commit into from
Jul 5, 2021

Conversation

Skolotsky
Copy link
Collaborator

Introduce css animations like this

object AppStyleSheet : StyleSheet() {
   val bounce by keyframes {
        from {
            property("transform", "translateX(50%)")
        }

        to {
            property("transform", "translateX(-50%)")
        }
    }

    val myClass by style {
        animation(bounce) {
            duration(2.s)
            timingFunction(AnimationTimingFunction.EaseIn)
            direction(AnimationDirection.Alternate)
        }
    }
}

@Skolotsky Skolotsky requested review from Schahen and eymar June 23, 2021 15:54
@@ -237,3 +237,70 @@ fun StyleBuilder.padding(value: CSSNumeric) {
// padding hasn't Typed OM yet
property("padding", value)
}

@Suppress("EqualsOrHashCode")
data class CSSAnimation(
Copy link
Member

Choose a reason for hiding this comment

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

I see that you usually avoid using data classes.
Is it necessary in this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is necessary due to equals check. We discussed with @Schahen that later we will think about replacing data classes with some interface which needs equals to be implemented.

@@ -39,17 +40,21 @@ class CSSRulesHolderState : CSSRulesHolder {
* ```
*/
open class StyleSheet(
private val rulesHolder: CSSRulesHolder = CSSRulesHolderState()
private val rulesHolder: CSSRulesHolder = CSSRulesHolderState(),
val usePrefix: Boolean = true,
Copy link
Member

Choose a reason for hiding this comment

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

I didn't find/see any cases when usePrefix = false.
What's the idea behind this parameter? When would one want to pass false?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's for the cases when user want integrate with some third party lib and it's classes.

}
}

inline fun CSSAnimation.duration(vararg values: CSSSizeValue<out CSSUnitTime>) {
Copy link
Member

@eymar eymar Jun 29, 2021

Choose a reason for hiding this comment

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

There is a list of functions that modify CSSAnimation state:
duration, timingFunction, delay, etc...
Are they intentionally made as extension functions, instead of adding them in the CSSAnimation class?
Could they be in the class instead?

2nd question:
Would it be useful to make this functions return CSSAnimation instance? then this function's calls can be chained: animation.delay(...).duration(...) What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was intentionally because it isn't contract, just convenient way to modify structure. Ant it is very similar to how other properties added to CSS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You don't need chain here because it's intended to be called in

animation(keyframe) {
   delay()
   duration()
}

So it's just a way how we are doing our DSL

@Skolotsky Skolotsky merged commit e2dd256 into master Jul 5, 2021
@Skolotsky Skolotsky deleted the feature/cssAnimations branch July 5, 2021 07:51
mareklangiewicz pushed a commit to mareklangiewicz/compose-jb that referenced this pull request Feb 14, 2022
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