CSS: Support custom properties #3557
Conversation
|
|
| ( value = hooks.set( elem, value, extra ) ) !== undefined ) { | ||
|
|
||
| style[ name ] = value; | ||
| if ( isCustomProp ) { |
There was a problem hiding this comment.
I originally planned to remove it as discussed in #3199 to fix https://bugs.jquery.com/ticket/14394 but CSSStyleDeclaration#setProperty requires styles in the kebab-case format so whatever we decide about that I'd like it to land separately.
There was a problem hiding this comment.
I'm fine with breaking that out for a later fix, it does seem like all of this can be refactored better at that time.
src/css.js
Outdated
| // want to modify the value if it is a CSS custom property | ||
| // since they are user-defined. | ||
| if ( !isCustomProp ) { | ||
| name = jQuery.cssProps[ origName ] || |
There was a problem hiding this comment.
Maybe we could simplify this logic with usual if..else or move it to the helper method? Like getName or something?
There was a problem hiding this comment.
This logic is used only in jQuery.style & jQuery.css which have some duplicated code anyway. We have an issue to unify them (#2297) so this helper method would be used only once again when it's done.
There was a problem hiding this comment.
I would start now at least for the affected code, that issue is open for two years now
There was a problem hiding this comment.
I can extract it but won't we want to de-extract it back once those two methods get unified?
There was a problem hiding this comment.
I just don't like this
name = jQuery.cssProps[ origName ] ||
( jQuery.cssProps[ origName ] = vendorPropName( origName ) || origName );thingy :). Really hard to read
There was a problem hiding this comment.
Ah, right. I do agree with that. And if you refactor it into if-elses then it get long enough that it makes sense to extract it, I get it now. On it.
test/unit/css.js
Outdated
|
|
||
| assert.equal( $elem.css( "--prop1" ), "val1", "Basic CSS custom property" ); | ||
|
|
||
| // Support: Chrome 55, Safari 9.1-10.0 only |
There was a problem hiding this comment.
I forgot to remove Chrome from here.
Fixes jquerygh-3144 Closes jquerygh-3199 Closes jquerygh-3557
|
PR updated. I hope you like the changes, @markelog. :-) |
Fixes jquerygh-3144 Closes jquerygh-3199 Closes jquerygh-3557
|
Rebased so that it can be tested in AMD mode. |
|
@timmywil the PR changed a little since you last looked at it so PTAL. |
| ( value = hooks.set( elem, value, extra ) ) !== undefined ) { | ||
|
|
||
| style[ name ] = value; | ||
| if ( isCustomProp ) { |
There was a problem hiding this comment.
I'm fine with breaking that out for a later fix, it does seem like all of this can be refactored better at that time.
| // | ||
| // getPropertyValue is needed for: | ||
| // .css('filter') (#12537) | ||
| // .css('--customProperty) (#3144) |
There was a problem hiding this comment.
Doesn't the IE 9 mention above look like it referred to the whole comment?
There was a problem hiding this comment.
Maybe the support comment should be removed in favor of // .css('filter') in IE 9 only (#12537) as we won't be able to remove this code path anyway due to CSS vars.
There was a problem hiding this comment.
Yeah i was looking at the added lines and not thinking about it seeming to be related to IE9. I'd be fine with removing the // Support: comment since like you say it's no longer just an IE9 requirement.
There was a problem hiding this comment.
PR updated. Does it look OK now?
Fixes jquerygh-3144 Closes jquerygh-3199 Closes jquerygh-3557
5 participants
Fixes gh-3144
Closes gh-3199
Summary
This PR adds suppport for CSS Custom Properties (a.k.a. CSS Variables). This is a finalized version of the PR #3199 with passing tests.
Checklist
Mark an
[x]for completed items, if you're not sure leave them unchecked and we can assist.If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.comThanks! Bots and humans will be around shortly to check it out.