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

Support full syntax in function rules and values #682

Closed
10 tasks done
kof opened this issue Feb 12, 2018 · 22 comments
Closed
10 tasks done

Support full syntax in function rules and values #682

kof opened this issue Feb 12, 2018 · 22 comments
Assignees
Labels
complexity:high Best brains need to talk about it. feature request This will safe many lifes! important The thing you do when you wake up!

Comments

@kof
Copy link
Member

kof commented Feb 12, 2018

Currently the only plugin that we run over function values/rules is jss-default-unit. The problem with running every plugin over it is that we would add performance overhead to something that is supposed to be usable for animations.

Originates in https://github.com/cssinjs/react-jss/issues/185

Cases:

  • fallbacks in fn rules
  • media queries inside of function rules
  • media query as a fn rule nested
  • function values inside of media rules
  • jss-nested inside of fn rules
  • jss-nested as a fn rule
  • jss-global should support function values and rules
  • jss-compose should support linking function rules
  • jss-compose should support linking rules with fn values
  • jss.use(pluginObservable({process: false})), test for disabling plugins
@kof kof added bug It went crazy and killed everyone. complexity:high Best brains need to talk about it. labels Feb 12, 2018
@kof kof added the plugin When unplugged is fine label Mar 19, 2018
@kof kof changed the title [jss-expand] Support function rules and values for jss-expand [jss-expand][jss-nested] Support function rules and values Apr 5, 2018
@kof
Copy link
Member Author

kof commented Jun 18, 2018

I got an idea what we can do here: due to the fact that other cssinjs libs have also functions and they don't expect those to be updatable at 60 FPS we can do this:

  1. We assume that mainstream use case doesn't need to update at 60FPS
  2. We allow all required plugins to react on the values/rules changes like they do for normal values
  3. Introduce a modified interface for "fast" functions:
{
  button: {
   // If value is an array and first item is a function, we got a function value.
    top: [props => props.top]
  }
}
{
   // If rule is an array and first item is a function, we got a fast function rule.
  button: [(props) => ({
    top: props.top
  })
}

Additionally we could allow passing options:

{
  button: {
   // Second array value is an options object.
    top: [props => props.top, {someOptions}]
  }
}

Another interface idea:

// This function name could be something else…
import {fast} from 'jss'
{
  // fast() will mark the function with a flag that allows to distinguish "fast" functions without plugins.
  button: fast((props) => ({
    top: props.top
  }))  
}    

I am open for any other interface suggestions.

@kof kof added important The thing you do when you wake up! feature request This will safe many lifes! and removed bug It went crazy and killed everyone. labels Jun 19, 2018
@kof kof changed the title [jss-expand][jss-nested] Support function rules and values Support full syntax in function rules and values Jun 19, 2018
@kof kof removed the plugin When unplugged is fine label Jun 19, 2018
@kavehsajjadi
Copy link

Fast functions being ones that require updates at 60fps?

@kof
Copy link
Member Author

kof commented Jun 20, 2018

Fast functions being ones which can be updated frequently using .update() method without being slow for animations or other types of frequent updates.

Basically current function values/rules implementation IS the "fast" version, which is why I couldn't add all plugins to the results they return, because that would slow them down.

So we need a way to use the entire JSS syntax for more "static" use cases, where static doesn't mean it can be done only once, but it shouldn't be updated frequently. This is the way styled-components is using them. At the same time I want to keep the "fast" version for frequent updates.

So we need to distinguish those 2 versions somehow.

@AleshaOleg
Copy link
Member

@kof I got another idea according to "fast" functions. Maybe better to revert your idea, and use such kind of wrapper for function values/rules only if needed to isolate block (class name) from root scope? Let's say this would be named as "slow" function. It will be executed separately with all of the plugins enabled by a user.

@kof
Copy link
Member Author

kof commented Sep 1, 2018

@AleshaOleg people expect all rules to work the same way with fn values, because other libraries support this. JSS is the only one with "fast" functions, thats why I think we should go by default for slow version and make fast optional. Another reason is that having animations is also not the most popular use cases compared to props based regular styling.

@HenriBeck HenriBeck mentioned this issue Sep 10, 2018
13 tasks
@kof
Copy link
Member Author

kof commented Sep 10, 2018

Idea: we can make function values/rules "slow" by default and only keep Observables as fast by default. The reasons are:

  1. Animations is not a very popular use case and it can still be done with passing an option to sheet.update()
  2. Observables is a nice interface and requires only a minimal polyfill to get started, one doesn't necessarily need a reactive library.

The benefits are

  1. There will be no confusion about fast/slow functions
  2. It will be easier to get typings done, since return value from function values/rules will be the same as the main declaration syntax.

That being said, we can reintroduce fast functions later on if there is a need as a new feature, since it requires a new interface now anyways.

@kof
Copy link
Member Author

kof commented Oct 14, 2018

Removed #474 and #796 from description, they require more work related to react-jss, too big for this task.

kof added a commit that referenced this issue Oct 14, 2018
* 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 #796, as part of #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

* Added tests for using jss-plugin-syntax-expand with function rules and values

* Skip fn values with jss-expand for now

* 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
kof added a commit that referenced this issue Oct 14, 2018
* 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 #796, as part of #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

* support typed cssom

* Update docs/json-api.md

* Update changelog.md
kof added a commit that referenced this issue Oct 16, 2018
* 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 #796, as part of #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

* Added tests for using jss-plugin-syntax-expand with function rules and values

* Skip fn values with jss-expand for now

* 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 changelog.md

* process: true always
kof added a commit that referenced this issue Oct 22, 2018
* 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 #796, as part of #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
@HenriBeck
Copy link
Member

Implemented in #878

@DurgaPammi

This comment has been minimized.

@HenriBeck

This comment has been minimized.

@DurgaPammi

This comment has been minimized.

@HenriBeck

This comment has been minimized.

@DurgaPammi

This comment has been minimized.

@JamesDonnelly
Copy link

Implemented in #878

When will this be made publicly available? There have been no releases since July. 😢

@kof
Copy link
Member Author

kof commented Nov 14, 2018

You can help doing it, otherwise just wait and watch issues, we are working on a big release.

bhupinderbola pushed a commit to bhupinderbola/jss that referenced this issue Sep 17, 2019
* 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

* Added tests for using jss-plugin-syntax-expand with function rules and values

* Skip fn values with jss-expand for now

* 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
bhupinderbola pushed a commit to bhupinderbola/jss that referenced this issue Sep 17, 2019
* 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

* support typed cssom

* Update docs/json-api.md

* Update changelog.md
bhupinderbola pushed a commit to bhupinderbola/jss that referenced this issue Sep 17, 2019
* 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
@monsieur-z
Copy link

Hi,

I'm using [email protected] and can't figure out how to make @global and dynamic values work together (I'm not sure this is the right thread, but similar issues points to this one).

I've been able to do it easily with jss only, like you can see in this pen: https://codesandbox.io/s/jss-dynamic-global-value-7n9rb

But I'm not able to achieve the same behaviour with react-jss: https://codesandbox.io/s/react-jss-dynamic-global-value-j552b. I've checked the doc: am I missing something obvious?

I've found a workaround by importing the jss instance (import { jss } from "react-jss";), but this doesn't seem right.

Thanks! I love this project.

@BerndWessels
Copy link

@monsieur-z have you found a way?

@alifsabrani
Copy link

Hi,

I'm using [email protected] and can't figure out how to make @global and dynamic values work together (I'm not sure this is the right thread, but similar issues points to this one).

I've been able to do it easily with jss only, like you can see in this pen: https://codesandbox.io/s/jss-dynamic-global-value-7n9rb

But I'm not able to achieve the same behaviour with react-jss: https://codesandbox.io/s/react-jss-dynamic-global-value-j552b. I've checked the doc: am I missing something obvious?

I've found a workaround by importing the jss instance (import { jss } from "react-jss";), but this doesn't seem right.

Thanks! I love this project.

Can you explain more about your workaround using jss instance? I encountered same issue and still can't find a solution

@monsieur-z
Copy link

There's no more to it than what's in this pen: https://codesandbox.io/s/jss-dynamic-global-value-7n9rb

@sam-dassana
Copy link

So I got dynamic global styles working by wrapping it in a function.

import { createUseStyles as createJssUseStyles } from 'react-jss'
import React from 'react'
import ReactDOM from 'react-dom'

const createUseStyles = ({ color }) => {
  return createJssUseStyles({
    test: {
      height: 30
    },
    // you need to have the local class outside of \@global and @global styles must come after
    // eslint-disable-next-line sort-keys
    '@global': {
      '.wrapper': {
        '& $test': {
          background: color
        }
      }
    }
  })
}

const Test = ({ color }) => {
  const classes = createUseStyles({ color })()

  return (
    <div className='wrapper'>
      <div className={classes.test}>TESTO</div>
    </div>
  )
}

Some observations:

  1. You need to have the local class outside of @global and @global styles must come after
  2. There needs to be at least ONE global class wrapping the local class.
  3. You need to write the classes in separate lines at least for the first two classes including @global. You can't do '@global': { '.wrapper & $test': {...}}}

@kof
Copy link
Member Author

kof commented Jan 27, 2021

@sam-m-m this is going to generate styles every time component updates, don't recommend doing this

@BM-BrunoMarques
Copy link

BM-BrunoMarques commented Aug 4, 2021

@sam-m-m this is going to generate styles every time component updates, don't recommend doing this

was ever a development in solving this issue of dynamic styles in global styling in react-jss?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complexity:high Best brains need to talk about it. feature request This will safe many lifes! important The thing you do when you wake up!
Projects
None yet
Development

No branches or pull requests