Skip to content

Always use insertRule API with option link: true #857

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

Merged
merged 15 commits into from
Sep 17, 2018
Merged

Always use insertRule API with option link: true #857

merged 15 commits into from
Sep 17, 2018

Conversation

kof
Copy link
Member

@kof kof commented Sep 16, 2018

What would you like to add/fix?

Current linking logic causes too many bugs. This approach simplifies it. We are not linking any more using selector, we create rules using insertRule API always when we need to link a rule.

Corresponding issue (if exists):

  • Need to test which currently open issues it fixes.
  • Need to check how linking works with nesting and container rules like media queries.

Fixes #815, #710, #664

@kof kof changed the title [WIP] Always use insertRule API with option link: true Always use insertRule API with option link: true Sep 16, 2018
@kof
Copy link
Member Author

kof commented Sep 16, 2018

This PR seems to be ready

@kof kof requested review from lttb, TrySound and HenriBeck September 16, 2018 10:46
@kof
Copy link
Member Author

kof commented Sep 16, 2018

Ok added a test with media query, its not ready, fixing it.

@@ -34,7 +31,6 @@ export default class StyleSheet {
constructor(styles: Object, options: StyleSheetOptions) {
this.attached = false
this.deployed = false
this.linked = false
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the linked property?

Copy link
Member Author

Choose a reason for hiding this comment

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

because we don't need it any more, if options.link is true, then it is always linked, there is no separate sheet.link() call.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah makes sense

@kof
Copy link
Member Author

kof commented Sep 16, 2018

Should fix (still verifying) #815, #710, #664
@TrySound @HenriBeck please review the last commit

@TrySound
Copy link
Member

Looks good to me

@HenriBeck
Copy link
Member

Yeah, to me too

@kof kof merged commit 3163a7c into master Sep 17, 2018
@kof
Copy link
Member Author

kof commented Sep 17, 2018

Merged

@kof kof deleted the remove-link branch October 14, 2018 22:15
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.

"Rule is not linked" error for function rules with non-canonical selector formatting
3 participants