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

Keyframes refactoring #1930

Merged
merged 2 commits into from Aug 21, 2018

Conversation

Projects
None yet
6 participants
@Fer0x
Contributor

Fer0x commented Aug 20, 2018

Re-implement code from #1700 which ideas was taken from #1496

Based on current develop branch (I hope to ship this with 4.0 together with new global styling API)

Changelog:

  • keyframes`` helper now returns Keyframes model
  • keyframes styles now inserting only with component rendering
  • To get animation name use .getName() method
  • some tests was changed to proper work with new keyframes helper
Implement lazy keyframes
- keyframes`` helper  now returns Keyframes model
- keyframes styles now inserting only with component rendering
- To get animation name use `.getName()` method
- some tests was changed to proper work with new `keyframes` helper

@Fer0x Fer0x changed the base branch from master to develop Aug 20, 2018

@Fer0x Fer0x referenced this pull request Aug 20, 2018

Closed

Lazy keyframes injection #1700

0 of 2 tasks complete

@Fer0x Fer0x requested review from kitten and probablyup Aug 20, 2018

@Fer0x

This comment has been minimized.

Contributor

Fer0x commented Aug 20, 2018

This PR has breaking changes with return value of keyframes helper. My previous thoughts was to add .toString() method in order to achieve backward compatibility. But coercion may cause very unpredictable results, like using typeof or strict equal. So, here now method .getName() which can give actual name of animation.
Eventually it's not a big change, I don't think too many people used this feature.

}
}
getName() {

This comment has been minimized.

@kitten

kitten Aug 20, 2018

Member

Just an idea; if we implement “valueOf” and “toString” to inject the keyframes CSS, then the effects of this breaking change would be minimal :) cc @probablyup

This comment has been minimized.

@Fer0x

Fer0x Aug 20, 2018

Contributor

I wrote about it #1930 (comment)

And in previous PR you said that explicit coercion is better than implicit ;)

This comment has been minimized.

@mxstbr

mxstbr Aug 20, 2018

Member

Honestly, I feel like it's fine for this to be a breaking change in v4—let's not do too much hacky stuff?

This comment has been minimized.

@kitten

kitten Aug 20, 2018

Member

Sorry, just thinking about breaking changes again, since we already have quite a lot 😅 In this case, just refer to my old comment lol. Sorry about that.

@mxstbr @probablyup If we can get this change in, we can probably deprecate a couple of old uses of the StyleSheet and get rid of cloning soon, since we won’t have non-component usages anymore

@kitten

kitten approved these changes Aug 20, 2018

Looking awesome apart from the missing micro optimisation 👌

return ruleSet.concat(chunk.getName())
}
return ruleSet.concat(chunk)

This comment has been minimized.

@kitten

kitten Aug 20, 2018

Member

Looking at the function interpolation above, it seems that calling push and returning ruleSet is equally save and it has probably been changed by @probablyup as a micro optimisation, so let’s do the same here :)

@kitten

kitten approved these changes Aug 20, 2018

@mxstbr

mxstbr approved these changes Aug 20, 2018

You are the best, thank you so much for tackling this!

.sc-b { } .c { -webkit-animation:keyframe_144 1s both; animation:keyframe_144 1s both; }
@-webkit-keyframes keyframe_144 {from {opacity:1;}}@keyframes keyframe_144 {from {opacity:1;}}

This comment has been minimized.

@probablyup

probablyup Aug 20, 2018

Contributor

Is it a problem that this is injected after the usage?

This comment has been minimized.

@Fer0x

Fer0x Aug 20, 2018

Contributor

@probablyup Look at example 3 https://www.w3.org/TR/css-animations-1/#at-ruledef-keyframes

It's ok to define keyframes elsewhere from it usage.

@probablyup

Let's do it!

@probablyup probablyup merged commit 45c8fc7 into develop Aug 21, 2018

2 checks passed

bundlesize ./dist/styled-components.min.js: 15.25KB < maxSize 16KB (gzip)(1.01KB smaller than master, good job!)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@probablyup probablyup deleted the keyframes-refactoring branch Aug 21, 2018

@probablyup

This comment has been minimized.

Contributor

probablyup commented Aug 21, 2018

@Fer0x mind doing a quick followup PR to add a changelog entry for your efforts? :)

@Fer0x

This comment has been minimized.

Contributor

Fer0x commented Aug 21, 2018

@probablyup #1935
Done, but I'm afraid that my English can blow someone's mind 😃

@Fer0x

This comment has been minimized.

Contributor

Fer0x commented Aug 21, 2018

@probablyup @mxstbr @kitten Also, these changes need to be described on the website.
Does website repo have something like 4.0 branch?

@stonebk

This comment has been minimized.

stonebk commented Sep 5, 2018

I've been trying out the 4.0 beta, and it appears keyframes break when you try to use them in a nested template string:

const fadeIn = keyframes`
    0% {                                                                                                                                                                                                                                    
        opacity: 0;                                                                                                                                                                                                                         
    }                                                                                                                                                                                                                                       
    100% {                                                                                                                                                                                                                                  
        opacity: 1;                                                                                                                                                                                                                         
    } 
`;

const Foo = styled.div`
    ${props => props.enableAnimations ? `animation: ${fadeIn} 1s linear;` : ''}
`;

This ends up adding the animation: animation: [object Object] 1s linear;

Is this expected behavior?

I have worked around it by putting the animation in a separate css like so:

const fadeIn = keyframes`
    0% {                                                                                                                                                                                                                                    
        opacity: 0;                                                                                                                                                                                                                         
    }                                                                                                                                                                                                                                       
    100% {                                                                                                                                                                                                                                  
        opacity: 1;                                                                                                                                                                                                                         
    } 
`;

const fadeInAnimation = css`
    animation: ${fadeIn} 1s linear;
`;

const Foo = styled.div`
    ${props => props.enableAnimations ? fadeInAnimation : ''}
`;
@probablyup

This comment has been minimized.

Contributor

probablyup commented Sep 5, 2018

@probablyup

This comment has been minimized.

Contributor

probablyup commented Sep 5, 2018

@alansouzati

This comment has been minimized.

alansouzati commented Sep 10, 2018

is it a good idea to list this as a breaking changing here? https://www.styled-components.com/docs/faqs#what-do-i-need-to-do-to-migrate-to-v4

@probablyup

This comment has been minimized.

Contributor

probablyup commented Sep 10, 2018

@alansouzati

This comment has been minimized.

alansouzati commented Sep 10, 2018

I don't think so, I'm in the process of migrating and if I change nothing in my code and use styled-components v3 I don't get animation: [object Object] 1s linear; now I'm getting it. Let me try to put two codesandboxes together.

@alansouzati

This comment has been minimized.

alansouzati commented Sep 10, 2018

Here is a working example using styled-components v3:

https://codesandbox.io/s/3qrz117n95

Here is the same example using styled-components v4 (does not animate):

https://codesandbox.io/s/nrq5owl63l

@alansouzati

This comment has been minimized.

alansouzati commented Sep 10, 2018

I'm my case it is slightly tricker to move things around to make it work, since the animations are dynamic and I use props to generate the final animation string.

@alansouzati

This comment has been minimized.

alansouzati commented Sep 10, 2018

for example I can pass an array of animation names to a component and it will generate multiple keyframes calls.

@alansouzati

This comment has been minimized.

alansouzati commented Sep 10, 2018

Here is a more complex example on what I'm trying to achieve:

Working fine with v3: https://codesandbox.io/s/0o8k69vlow

Broken in v4: https://codesandbox.io/s/ll1o5xwqkl

@probablyup

This comment has been minimized.

Contributor

probablyup commented Sep 10, 2018

Okay, I think this was working before on a technicality but wasn't explicitly documented so yeah I'll update the migration guide to call it out. In general we recommend always using the css helper when creating CSS partials with more than a simple value to be interpolated into a styled component.

@probablyup

This comment has been minimized.

Contributor

probablyup commented Sep 10, 2018

@alansouzati and here's your v4 example tweaked with the css helper to get it working: https://codesandbox.io/s/74765zlom6

@alansouzati

This comment has been minimized.

alansouzati commented Sep 10, 2018

How do you solve for this example? I’m failing to identify how to put this together for arrays

https://codesandbox.io/s/ll1o5xwqkl

I would appreciate if you fork this and try to make this work in v4

@probablyup

This comment has been minimized.

Contributor

probablyup commented Sep 10, 2018

@alansouzati

This comment has been minimized.

alansouzati commented Sep 10, 2018

yeah I thought about using reduce as well. the code readability is slightly reduced here. But I understand the refactor. thanks for chiming in so quick.

@alansouzati

This comment has been minimized.

alansouzati commented Sep 17, 2018

hey I think something is up with the latest beta 5 v4 release.

Take a look at this codesandbox (it does not fade if I put the opacity in a variable):

https://codesandbox.io/s/m4116pw31p

Using beta 1, this works fine, check this codesandbox:

https://codesandbox.io/s/jv3q8z9o65

@Fer0x

This comment has been minimized.

Contributor

Fer0x commented Sep 17, 2018

@alansouzati @probablyup
This can be happened after #2006

@probablyup

This comment has been minimized.

Contributor

probablyup commented Sep 17, 2018

Thanks taking a look

@alansouzati

This comment has been minimized.

alansouzati commented Sep 17, 2018

thanks for chiming in @Fer0x

on a side note, a work around was to move that interpolation to a separate variable, and use css from styled-components...

check this codesandbox:

https://codesandbox.io/s/o426lv949y

@probablyup

This comment has been minimized.

Contributor

probablyup commented Sep 17, 2018

This should fix it: #2013

Sorry about that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment