Skip to content

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.

2 participants