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

make rule-value-function and global work together #1427

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

Conversation

louisabraham
Copy link

@louisabraham louisabraham commented Nov 14, 2020

Fix #1335

Implementation details

I attach an attribute updateFun to rules with type 'global' that have a function as declaration (i.e. rules that match both plugins).

I already had to modify RuleList.js to prevent the special case rule.rules instanceof RuleList from skipping the update, so I included the code that executes updateFun in RuleList.js. An important choice (that is easily modifiable) is that plugins.onUpdate will not be launched on the global rule that is updated with a function: other plugins will not run on the updated properties.

Another meaningful possibility would be to include the call to updateFun in the onUpdate method of the rule-value-function. This would allow us to call plugins.onUpdate from RuleList.js, so have other plugins run on the rule after update. I was not sure what interactions this could create with other plugins so I didn't handle it this way.

It is my first contribution to a JS project, so please feel free to give me advice to improve my coding style 😃

Example

import jss, {SheetsRegistry} from 'jss'
import funplugin from 'jss-plugin-rule-value-function'
import globplugin from 'jss-plugin-global'

const jss_ = jss.default

jss_.use(funplugin.default(), globplugin.default())

const sheet = jss_.createStyleSheet({
  '@global': (data) => ({
    button: {
      width: 100,
      height: 100
    },
    '#sntch_block': {
      display: data.display
    }
  }),

  a: {
    display: 'inline',
    color: (data) => data.color
  },

  div: (data) => ({
    color: data.color
  })
})

const sheets = new SheetsRegistry()
sheets.add(sheet)
var str = sheets.toString()
console.log(str)

console.log('\n-----\n')

sheet.update({color: 'red', display: 'none'})

str = sheets.toString()
console.log(str)

console.log('\n-----\n')

sheet.update({color: 'blue', display: 'none'})

str = sheets.toString()
console.log(str)

gives

.a-0-0-1 {
  display: inline;
}

-----

button {
  width: 100;
  height: 100;
}
#sntch_block {
  display: none;
}
.a-0-0-1 {
  display: inline;
  color: red;
}
.div-0-0-2 {
  color: red;
}

-----

button {
  width: 100;
  height: 100;
}
#sntch_block {
  display: none;
}
.a-0-0-1 {
  display: inline;
  color: blue;
}
.div-0-0-2 {
  color: blue;
}

@louisabraham
Copy link
Author

louisabraham commented Nov 14, 2020

I don't know how to add tests, but I think it would be a nice thing if someone has a bit of time!

(also it looks that there are issues in master that prevent the tests from running)

@kof
Copy link
Member

kof commented Nov 16, 2020

See #1335 (comment) regarding the syntax.

// It is a rules container like for e.g. ConditionalRule.
if (rule.rules instanceof RuleList) {
// Ignore if there is an updateFun
if (!hasUpdateFun && rule.rules instanceof RuleList) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

breaks encapsulation between plugins and the core, core has no knowledge about style functions, so far we managed to run the plugin in a way that doesn't require this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the idea is that the plugin reacts to onUpdate hook and calls the function

@@ -22,7 +23,19 @@ export default function functionPlugin() {
onCreateRule(name?: string, decl: JssStyle, options: RuleOptions): Rule | null {
if (typeof decl !== 'function') return null
const rule: StyleRuleWithRuleFunction = (createRule(name, {}, options): any)
rule[fnRuleNs] = decl
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was how we hide the implementation detail of the interface that only this plugin knows how to handle

Copy link
Member

@kof kof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for the PR, we def. need to change the implementation

@kof kof added the needs tests We can't merge a change without tests verifying it's behaviour label Jun 27, 2021
@kof
Copy link
Member

kof commented Jun 27, 2021

Would be great if someone added tests to rule-value-function package, so we can play with the implementation details and find a better way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted needs tests We can't merge a change without tests verifying it's behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Variable values in @global
2 participants