Skip to content

Fix addRule for functional values, fix #475#477

Merged
kof merged 2 commits intocssinjs:masterfrom
lttb:feature/fix-add-rule-for-fn
Apr 26, 2017
Merged

Fix addRule for functional values, fix #475#477
kof merged 2 commits intocssinjs:masterfrom
lttb:feature/fix-add-rule-for-fn

Conversation

@lttb
Copy link
Copy Markdown
Member

@lttb lttb commented Apr 25, 2017

The problem is that rule.toString() returns nothing if we have just function values in rule without calculated values. So it needs to return just an empty rule like .class {} to be applied to the stylesheet.

@kof
Copy link
Copy Markdown
Member

kof commented Apr 25, 2017

Why do you call toString() before .update()?

@lttb
Copy link
Copy Markdown
Member Author

lttb commented Apr 25, 2017

We call .toString in injectRule, that called in addRule. and we make .addRule before .update

@kof
Copy link
Copy Markdown
Member

kof commented Apr 25, 2017

Maybe we should change the behavior there?

@kof
Copy link
Copy Markdown
Member

kof commented Apr 25, 2017

I am not sure what will happen if we render a rule without props in different browsers.

@lttb
Copy link
Copy Markdown
Member Author

lttb commented Apr 25, 2017

But we can add rule without data (for lazy apply for example), and it's a normal behavior for user

@kof
Copy link
Copy Markdown
Member

kof commented Apr 25, 2017

Yeah, need to run tests in all browsers.

@lttb
Copy link
Copy Markdown
Member Author

lttb commented Apr 25, 2017

if we will have some issues with browsers, we always can render an empty prop

@kof
Copy link
Copy Markdown
Member

kof commented Apr 25, 2017

what is an "empty" prop?

@lttb
Copy link
Copy Markdown
Member Author

lttb commented Apr 25, 2017

what is an "empty" prop?

just an empty string, for example .class {color:''}
or we can also make it initial

@kof
Copy link
Copy Markdown
Member

kof commented Apr 25, 2017

color: '' is an invalid value, I don't know what this might cause.

@kof
Copy link
Copy Markdown
Member

kof commented Apr 25, 2017

"initial" is an actual value which might lead to unexpected results from the user perspective

@iamstarkov
Copy link
Copy Markdown
Member

iamstarkov commented Apr 25, 2017

color's initial value is different in every browser

Comment thread tests/functional/sheet.js Outdated
})
})

/*
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why is this commented out?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

sorry, just forgot to uncomment

Comment thread src/utils/toCss.js Outdated
}

if (!result) return result
if (!(result || hasFunctionValue)) return result
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

much more readable:

if (!result && !hasFunctionValue) return result

@lttb lttb force-pushed the feature/fix-add-rule-for-fn branch from 4ebaee9 to b9e3da2 Compare April 25, 2017 21:51
@kof
Copy link
Copy Markdown
Member

kof commented Apr 26, 2017

some browsers time out on browserstack, trying to figure out why.

@kof kof merged commit acc9410 into cssinjs:master Apr 26, 2017
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.

3 participants