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

Add vendor prefixes to keyframes #106

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

Conversation

DylanArnold
Copy link

No description provided.

@DylanArnold
Copy link
Author

BTW I was in a rush to implement this so didn't know of an alternative implementation (like not hard coding the prefixes). It seemed like the existing keyframes implementation was a special case though.

@japgolly
Copy link
Owner

Hey @DylanArnold, I meant to reply to you but lost forgot. Sorry about that!

We already have a mechanism in place for adding prefixes, here is an example of key transforms like you're doing:
https://github.com/japgolly/scalacss/blob/master/core/shared/src/main/scala/scalacss/internal/Attrs.scala#L121

And it can even transform values like this:
https://github.com/japgolly/scalacss/blob/master/core/shared/src/main/scala/scalacss/internal/Attrs.scala#L578-L579

I imagine it would be a similar one-line declaration to add prefixes to keyframes.

@DylanArnold
Copy link
Author

DylanArnold commented Jan 8, 2017

@japgolly I've been looking at how to implement this but it's a little bit tricky.

I get that I can do this

val attr = Attr.real("keyframes", Transform keys CanIUse.animation)
val result: Vector[CssKV] = attr.gen(Env.empty)("x")

The first problem is that attr.gen needs a value.

First of all I figured that I'll just build the internal content of keyframes then pass that as the value to attr.gen.

The problem is that each of of the functions passed to FormatSB side effect and mutate the StringBuilder.

I can see two options straight off.

  1. Easiest but a little bit hacky. Similar to what my commit contains.

Instead of

Seq("-webkit-", "-moz-", "-o-", "").foreach { p => 
    kfStart(p, e.name.value)
    for ((sel, styles) <- e.frames) {
        kfsStart(sel.value)
        val selO = Some(sel)
        for (s <- styles)
             printCssKV(selO, s.mq, s.content)
             kfsEnd(sel)
    }
    kfEnd(e.name)
}

Becomes

val attr = Attr.real("keyframes", Transform keys CanIUse.animation)
val result: Vector[CssKV] = attr.gen(Env.empty)("x")

result.foreach { kv => 
    kfStart(kv.key, e.name.value)
    for ((sel, styles) <- e.frames) {
        kfsStart(sel.value)
        val selO = Some(sel)
        for (s <- styles)
             printCssKV(selO, s.mq, s.content)
             kfsEnd(sel)
    }
    kfEnd(e.name)
}

So the initial "x" value passed to attr.gen is discarded.

Option 2. I'm not so sure. I think it would require some refactoring but it's hard to tell the best way since I didn't write this. Do you have any thoughts? I could look into this more. One option could be passing the StringBuilder around to each function in FormatSB, this might allow more fine grained rendering, and allow a nested StringBuilder to be created for the KeyFrames rendering and the result passed to attr.gen. Still, I don't think this option quite fits into the way it is currently structured.

Or something like that. That's as far as my thought process got. IMO currently the quick and dirty Option 1 seems reasonable :p

@japgolly
Copy link
Owner

@DylanArnold Hey. I'm not totally against quick and dirty as long as it's not observable externally. But before I merge (or suggest a different course like your option 2) I was planning to jump back into the code and investigate. Unfortunately I haven't been able to yet and head up: won't until start of Feb. Sorry.

@DylanArnold
Copy link
Author

DylanArnold commented Jan 17, 2017

Hey. No problem I'm working off a snapshot anyway.

Option 1 is better than the current commit and from what I can tell isn't actually that bad. Other options probably need some refactoring unless there is something I haven't seen yet,

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

2 participants