cssstyle
cssstyle copied to clipboard
fix: webkit properties not being recognized
Seems like they were never tested. I'm not sure if the two added tests can be fixed with independent commits.
Codecov Report
Merging #112 into master will decrease coverage by
0.31%
. The diff coverage is100%
.
@@ Coverage Diff @@
## master #112 +/- ##
==========================================
- Coverage 37.39% 37.07% -0.32%
==========================================
Files 87 87
Lines 1182 1176 -6
Branches 227 225 -2
==========================================
- Hits 442 436 -6
Misses 633 633
Partials 107 107
Impacted Files | Coverage Δ | |
---|---|---|
lib/allWebkitProperties.js | 100% <100%> (ø) |
:arrow_up: |
lib/parsers.js | 80.35% <0%> (-0.3%) |
:arrow_down: |
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 62b3b1b...c01df24. Read the comment docs.
@eps1lon is this a regression we missed in the last PR that ended up being 2.2.0 - or new functionality?
@eps1lon is this a regression we missed in the last PR that ended up being 2.2.0 - or new functionality?
Tried the tests in 2.1.0 (git checkout v2.1.0
) and they were failing as well. I couldn't find any tests related to webkit properties. I can try to create some tests related to webkit properties that are passing in 2.1.0.
Only webkit properties are in the spec; other engine prefixes should not be added.
Only webkit properties are in the spec; other engine prefixes should not be added.
This statement clashes with
https://github.com/jsdom/cssstyle/blob/ebd2dab41a3396419513a7698cdd37494727d0da/lib/allExtraProperties.js#L4-L5
@jsakas Is there anything I can do to get this merged?
@eps1lon sorry for the delay here. Are you able to resolve the conflict for me? Also, based on @domenic's comment I think only webkit should be supported at this time.
Are you able to resolve the conflict for me?
Sure thing.
Also, based on @domenic's comment I think only webkit should be supported at this time.
Personally I would prefer if this also handled other prefixed properties. This is in line with https://github.com/jsdom/cssstyle/blob/ebd2dab41a3396419513a7698cdd37494727d0da/lib/allExtraProperties.js#L4-L5
But I understand if you want to change that stance to "we only support properties in the spec".
Hi @eps1lon, do you intend to fix conflict for this pull request? I encounter an error with a webkit CSS property not being recognized correctly, and this issue should fix it. I you're not willing to do it, I'll be happy to do it.
What I also noticed is that webkit specific value are accessible through 2 names:
webkitTextFillColor and WebkitTextFillColor
I'm currently not sure if this PR will implement both of them (test case looks like it is)
BTW allProperties define only 1 webkit property alone, -webkit-line-clamp
I think this PR should remove this line, introduced lately on January
Not working on this one at the moment