Scoped keyframes by kof · Pull Request #888 · cssinjs/jss

kof · 2018-10-13T11:50:39Z

- move documentation to the package - update docs, since plugins don't apply by default - introduce option process: true to the plugin to enable plugins if user wants that

kof · 2018-10-13T11:54:56Z

+    // TODO make it more robust
+    this.name = key.substr(this.type.length + 2)
+    const fakeRule: Rule = ({key: this.name}: any)
+    this.id = options.jss.generateClassName(fakeRule, options.sheet)


this is not very nice, I actually want id generation to be outside of a rule and passed to it

We could rename generateClassName to generateUniqueId?

…d values

kof · 2018-10-13T14:57:32Z

Thought about this too as well as about a new interface. The problem here is that key in this rule unusable for id, so I had to create a fake rule. Alternatively the key can be changed to something usable for id, then interface can stay.

…

On Sat, Oct 13, 2018, 16:22 Henri ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/jss/src/rules/KeyframesRule.js <#888 (comment)>: > @@ -20,6 +28,10 @@ export default class KeyframesRule implements ContainerRule { constructor(key: string, frames: Object, options: RuleOptions) { this.key = key + // TODO make it more robust + this.name = key.substr(this.type.length + 2) + const fakeRule: Rule = ({key: this.name}: any) + this.id = options.jss.generateClassName(fakeRule, options.sheet) We could rename generateClassName to generateUniqueId? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#888 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADOWMMY3AX7-6s8hzw5eq7s61O6_NoFks5ukfc4gaJpZM4Xal7d> .

HenriBeck · 2018-10-13T15:56:21Z

 })

+// Animation name ref replacer.
+plugins.push({


Not a fan of this. I think we should rename all of the Rules to StylePlugin etc. and move this functionality to the KeyframesPlugin.

yeah, that would be cleaner, but also more hooks and more code, not sure its worth it

I think it's worth it.

HenriBeck · 2018-10-13T15:57:02Z

+    if (rule.type !== 'style' || !sheet) return style
+
+    // We need to support camel case here, because this plugin runs before the camelization plugin.
+    const prop = 'animationName' in style ? 'animationName' : 'animation-name'


I think we also need to support animation: { name: 'fadeIn' }

and also the animation: $keyframeId 150ms shortcut

thought about it an decided to limit it for now, because it goes into difficult directions, adds more code and slows down.

In the first case you would introduce the knowledge from jss-expand plugin about new syntax, in the second case, you would have to always run a regex or indexOf on the animation property.

We already had to introduce camel case support in the core, which is suboptimal. In theory we could make this refs replacement a separate plugin, but then its not very logical that one can't reference keyframes from the same sheet without plugins.

Making it a separate plugin and running after jss-expand and jss-camel-case, would allow to just support "animation-name" property and rely on the animation: {name} and animationName being converted to animation-name

Another thought I had a while ago: what if we extract all core rules into separate packages and plugins? Most people use preset anyways. A few people can also setup by themselves all the plugins. This would give us the flexibility with above mentioned plugin.

Additional benefit: someone who doesn't need all the rules, doesn't have to include them. For e.g. we got a request to include @page rule and there are some more rarely used rules. Why not make them all separate packages, but included in default preset.

Yeah, let's maybe make all of the rules plugins for now and then move them out maybe.

We could do a KeyframeRulePlugin and a KeyframeReferencePlugin then. I think this would be the best solution.

This should allow us to support all of the mentioned case above without needing to introduce anything.

by "make all of the rules plugins" you mean just jss/src/plugins or making them separate packages each?

lets do this in a separate PR after the other PRs are merged, too many conflicts otherwise

Okay, I think we need to apply the rule plugins last so that the keyframes name replace works on every possibility.

Changed the key to a form where it can be used directly as-is for id generation, so we don't have to fake the rule when using generateClassName

onProcessStyle is invoked on each .update, but in this case we only need to do this once, because fn values can't be added

kof · 2018-10-18T12:42:08Z

Yeah, or a default plugins registry maybe? This registry would be private, and those hooks would be called by the public registry.

Separate registries? This could work now, but not sure if it won't be a problem later, if we want to expose the registry for some reason.

kof · 2018-10-18T12:43:42Z

Also PluginsRegistry is called from many places, I would have to make 2 maps inside PluginsRegistry, but thats exactly what I did.

kof · 2018-10-18T12:44:33Z

I can sort on .use() call though, this allows to have one registry. Sorting on .use() call is possible because its not a hot call.

TODO - Rethink BaseStyleRule - Evtl move generateClassName to the rule models - Evtl rename generateClassName to generateId

kof · 2018-10-21T23:24:47Z

Ok, I think we can do the final CR and merge it.

…t/add-markdown-lint * origin/feat/add-markdown-lint: Scoped keyframes (#888)

* origin/add-ts-typings: Scoped keyframes (#888) Update readme.md (#897) Use CSSTOM in default unit plugin (#893) Added .nvmrc (#896) [WIP] DONT MERGE Fix ci (#895) Update ci (#894) Support plugins processing for observables by default (#892) Typed CSSOM values support (#887) Dynamic values full syntax (#878) [jss-plugin-cache] Use WeakMap for cache plugin (#890)

* added fallbacks test to fn rules * only call onChangeValue if plugins: true * enable removing props from fn rules * shorter syntax with coercion * wip full syntax support * restore browsers.json * changelog * move hook call to the core * isProcessed flag explanation * added tests to fn rules * remove media rule as a function test case * added a test for cssinjs#796, as part of cssinjs#682 * tests for compose plugin * observables - move documentation to the package - update docs, since plugins don't apply by default - introduce option process: true to the plugin to enable plugins if user wants that * move fn values docs * wording * scoped keyframes * Added tests for using jss-plugin-syntax-expand with function rules and values * Skip fn values with jss-expand for now * use "keyframes-{name}" as a key in KeyFramesRule Changed the key to a form where it can be used directly as-is for id generation, so we don't have to fake the rule when using generateClassName * keyframes name parser with validation * wip keyframes inside of global * move container rules update to the core * optimize onProcessStyle onProcessStyle is invoked on each .update, but in this case we only need to do this once, because fn values can't be added * put variables in the right scope * changelog * update tests with key keyframes name * Update changelog.md * Update docs/json-api.md * Update changelog.md * process: true always * support multiple whitespaces after keyframes identifier * move keyframe name generation to outside * call external plugins first we need to separate internal and external queues, queue 0 is external, 1 is internal and 1 is executed always after 0 * move keyframes rule to plugins * move StyleRule to a plugin Additionally stop rendering StyleRule when user passes a bad/unknown at-rule * move viewport rule to plugins * move simple rule to plugins * move FontFace rule * rename plugins to {rule}Rule schema * beter types for queue * Add support for animation property * optimize perf, handle arrays * fix tests, add more tests * Fix * Support arrays * remove array support * use global instead of window * always run stylerule plugin first, to avoid regexes of the at rules * ensure to always use style rule as a first rule in plugins flow started to complain with "Unused suppression comment." so I removed them * fix cache plugin * test raw rule registration order * rewrite internal plugins queue * better logic in plugins.use() * better types * better types * fix types * keyframe inside of global TODO - Rethink BaseStyleRule - Evtl move generateClassName to the rule models - Evtl rename generateClassName to generateId * default export in plugins * move id generator to models * update snapshots * fix circular deps * use CSSKeyframeRule * generateClassName -> generateId * added exampls with keyframes to global plugin * docs, small syntactic changes

kof added 19 commits September 18, 2018 12:01

added fallbacks test to fn rules

b90fe69

only call onChangeValue if plugins: true

2989e27

enable removing props from fn rules

dfbb767

shorter syntax with coercion

2296958

Merge branch 'master' into dynamic-values-full-syntax

bdd1ddf

wip full syntax support

421d42b

Merge branch 'master' into dynamic-values-full-syntax

901ae2f

restore browsers.json

79228d2

changelog

3583f32

move hook call to the core

9a6a1e0

isProcessed flag explanation

49cc07f

added tests to fn rules

7e5eca9

remove media rule as a function test case

abfafa8

added a test for #796, as part of #682

c5fdf05

tests for compose plugin

a70b3e2

observables

54f2b7f

- move documentation to the package - update docs, since plugins don't apply by default - introduce option process: true to the plugin to enable plugins if user wants that

move fn values docs

220bb42

wording

d5b2794

scoped keyframes

2c10b42

kof commented Oct 13, 2018

View reviewed changes

Added tests for using jss-plugin-syntax-expand with function rules an…

171902b

…d values

Skip fn values with jss-expand for now

0e53702

HenriBeck requested changes Oct 13, 2018

View reviewed changes

kof added 6 commits October 13, 2018 23:48

use "keyframes-{name}" as a key in KeyFramesRule

31ecf79

Changed the key to a form where it can be used directly as-is for id generation, so we don't have to fake the rule when using generateClassName

keyframes name parser with validation

938b48b

wip keyframes inside of global

eda061d

move container rules update to the core

efede57

optimize onProcessStyle

59f5ccc

onProcessStyle is invoked on each .update, but in this case we only need to do this once, because fn values can't be added

put variables in the right scope

b17c63d

kof added 5 commits October 18, 2018 22:57

rewrite internal plugins queue

0e0d049

better logic in plugins.use()

d20b9dd

better types

0a5b84c

better types

5dfd336

fix types

a2b3c43

kof force-pushed the scoped-keyframes branch from a17cfae to a2b3c43 Compare October 21, 2018 10:45

kof added 8 commits October 21, 2018 15:33

keyframe inside of global

0b652e6

TODO - Rethink BaseStyleRule - Evtl move generateClassName to the rule models - Evtl rename generateClassName to generateId

default export in plugins

a1e4e0d

move id generator to models

a885d32

update snapshots

ba59e1c

fix circular deps

6fdd38c

use CSSKeyframeRule

0edcbc3

generateClassName -> generateId

fff8026

added exampls with keyframes to global plugin

a4243ff

kof changed the title ~~[WIP] scoped keyframes~~ Scoped keyframes Oct 21, 2018

HenriBeck approved these changes Oct 22, 2018

View reviewed changes

docs, small syntactic changes

4ce3b61

kof merged commit fc3773f into master Oct 22, 2018

kof deleted the scoped-keyframes branch October 22, 2018 12:39

HenriBeck mentioned this pull request Oct 22, 2018

Generated keyframe id #346

Closed

9 tasks

HenriBeck pushed a commit that referenced this pull request Oct 22, 2018

Merge remote-tracking branch 'origin/feat/add-markdown-lint' into fea…

5d6e4fb

…t/add-markdown-lint * origin/feat/add-markdown-lint: Scoped keyframes (#888)

synchronos-t mentioned this pull request Feb 21, 2019

[jss-plugin-vendor-prefixer]: @keyframes rule errors because of a typo in onProcessRule() #1041

Closed

Uh oh!

Conversation

kof commented Oct 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kof commented Oct 13, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kof Oct 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kof Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kof commented Oct 18, 2018

Uh oh!

kof commented Oct 18, 2018

Uh oh!

kof commented Oct 18, 2018

Uh oh!

kof commented Oct 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kof Oct 13, 2018 •

edited

Loading

kof Oct 14, 2018 •

edited

Loading