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

Add CSSGroupingRule and CSSConditionRule #111

Merged
merged 1 commit into from
Jun 20, 2021

Conversation

jonathantneal
Copy link
Contributor

This change adds the CSSGroupingRule and CSSConditionRule constructors, and it establishes the prototype chain between these and CSSMediaRule and CSSSupportsRule.

For the updated functionality, it includes references to the appropriate specifications, and I did my best to keep the coding style in sync with the existing project. For instance, I preserved the styling where tabs are currently intermixed with spaces in CSSMediaRule.js, and I preserved the strategy of using new CSSOM.CSS... over Object.create (which is done to support for Firefox 3.6, perhaps?).

For the tests, I verified that, while one test is failing from the current main branch, all the other tests are still passing after this change. I included a comment similar to others for a currently untestable getter.

Resolves #105

@jonathantneal jonathantneal force-pushed the jn.add-css-grouping-rule branch from e332296 to f6a07be Compare June 14, 2021 18:49
@NV
Copy link
Owner

NV commented Jun 17, 2021

Looks good at the first glance. I'll take a closer look this coming weekend.

Thank you for matching the existing code style. It certainly shouldn't intermix tabs and spaces (most of the project uses tabs), but it should be addressed separately.

Could you include links to Jest and JSDOM issues that motivated you to write this pull request? You've mentioned it in https://twitter.com/jon_neal/status/1405245633049600008.

@NV
Copy link
Owner

NV commented Jun 20, 2021

... and I preserved the strategy of using new CSSOM.CSS... over Object.create (which is done to support for Firefox 3.6, perhaps?).

It should be changed to the modern class syntax, e.g. class CSSConditionRule extends CSSGroupingRule, but it's fine to do that in a separate commit. I don't intend to support 10-year-old browsers, it's just the sign that the repo hasn't been maintained.

@NV NV merged commit 6b6de70 into NV:master Jun 20, 2021
@NV
Copy link
Owner

NV commented Jun 20, 2021

Thank you for contributing, @jonathantneal! Your code carefully matched the existing codebase.

I just published https://www.npmjs.com/package/cssom/v/0.5.0.

@NV
Copy link
Owner

NV commented Jun 20, 2021

If you'd like to contribute any changes related to code style inconsistencies (e.g. indentation) or outdated patterns (new CSSOM.CSS...), I'll gladly take pull requests.

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.

CSSMediaRule should derive from CSSConditionRule
2 participants