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

Enable vars and nested calc&var #127

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

snowystinger
Copy link

This library already just passes calc's right through. I propose doing the same with var and nested calcs and nested vars. We don't need to resolve them, but I do need them not to be swallowed silently in my tests.

This library already just passes calc's right through. I propose doing the same with var and nested calcs and nested vars. We don't need to resolve them, but I do need them not to be swallowed silently in my tests.
@codecov-io
Copy link

Codecov Report

Merging #127 (6b0100e) into master (b527ed7) will increase coverage by 0.26%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #127      +/-   ##
==========================================
+ Coverage   37.39%   37.65%   +0.26%     
==========================================
  Files          87       87              
  Lines        1182     1187       +5     
  Branches      227      229       +2     
==========================================
+ Hits          442      447       +5     
  Misses        633      633              
  Partials      107      107              
Impacted Files Coverage Δ
lib/parsers.js 80.88% <100.00%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b527ed7...6b0100e. Read the comment docs.

@CharlyTorregrosa
Copy link

Hi! AFAIK this closes #125 Could be consider @jsakas ?

@hughes-ch
Copy link

Just stumbled across this pull request after trying to figure out why var() wasn't working.

Instead of defining a new VAR parser type, should CSSStyleDeclaration.setProperty be updated instead? There's already a check for custom properties which can be leveraged. I'd suggest doing this (the only change is to setting isCustomProperty):

CSSStyleDeclaration.prototype = {
...
  setProperty: function(name, value, priority) {
...
    var isCustomProperty = name.indexOf('--') === 0 ||
        (typeof value === 'string' && /^var\(--[-\w]+,?.*\)$/.test(value));
    if (isCustomProperty) {
      this._setProperty(name, value, priority);
      return;
    }
...
  },

  // Included for context (already implemented on master)
  _setProperty: function(name, value, priority) {
    if (value === undefined) {
      return;
    }
    if (value === null || value === '') {
      this.removeProperty(name);
      return;
    }
    if (this._values[name]) {
      // Property already exist. Overwrite it.
      var index = Array.prototype.indexOf.call(this, name);
      if (index < 0) {
        this[this._length] = name;
        this._length++;
      }
    } else {
      // New property.
      this[this._length] = name;
      this._length++;
    }
    this._values[name] = value;
    this._importants[name] = priority;
    this._onChange(this.cssText);
  },

As implemented in this pull request, only the color property will be able to use var(). Other properties (such as font-size) will still run into issues parsing it.

@snowystinger
Copy link
Author

Thanks for taking a look, I'll see if I can find some time to update and check if that works, authors of the repo should feel free to push to my fork as well if they get to it before I do

@effervescentia
Copy link

@domenic @jsakas I'd like to follow up here (and on this PR) since it appears that there are a number of missing features from cssstyle and jsdom that prevent CSS variable inheritance to be testable using JSDOM

here is a minimum-viable failure case using the vanilla-extract styling library which relies heavily on CSS variables to enable performant theming and dynamic styling

https://stackblitz.com/edit/node-x7idfl?file=Button.test.tsx

@filipkis

This comment was marked as spam.

@snowystinger
Copy link
Author

@hughes-ch thanks for the suggestion, I think that will work. I've updated the PR. Apologies for the long wait

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.

6 participants