-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Added support for deprecated addRule & removeRule methods #1515
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: ad0fa7f The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
this: CSSStyleSheet, | ||
selector: string, | ||
styleBlock: string, | ||
index?: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If index
is undefined, the default will be zero (the insertRule
default)
However addRule
default is cssRules.length https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleSheet/addRule#index
Could we supply this new default before passing it off to insertRule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
When supplying cssRules.length
win.CSSStyleSheet.prototype.addRule = function (
this: CSSStyleSheet,
selector: string,
styleBlock: string,
index: number = this.cssRules.length,
) {
const rule = `${selector} { ${styleBlock} }`;
return win.CSSStyleSheet.prototype.insertRule.apply(this, [rule, index]);
};
I am getting an error when running the test, specifically when deleting rules
DOMException: Failed to execute 'deleteRule' on 'CSSStyleSheet': The index provided (3) is larger than the maximum index (2).
Will investigate and update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up separating the tests since idx kept moving around when calling deleteRule and removeRule in sequence
We recently had a client on our fork who still uses addRule & removeRule (bad practice, I know, but we had to support 'em)
This PR add support for deprecated methods addRule & removeRule
Instead of writing duplicate code to support these or make some sort of an abstraction I just "transpiled" the props from addRule to what insertRule accepts and called the original insertRule or deleteRule accordingly – let you guys' methods handle the rest
Also expanded the test for capturing stylesheets to test this
LMK what you think