-
Notifications
You must be signed in to change notification settings - Fork 9
Conversation
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.
Thanks for the PR! One comment.
@mcanepa Hey, do you plan to finish the PR? |
I would like to, but I'm not sure what do exactly. I don't know what to keep and what to change for each version of IE |
@mcanepa We don't need to handle IE <8 so we just need The main issue with the PR right now is you modified an existing block applied to an older jQuery UI version: Does that clarify things? |
lib/themeroller.js
Outdated
return ( opacity / 100 ).toString().replace( /^0\./, "." ) + ';-ms-filter: "alpha(opacity=' + opacity + ')"'; | ||
}; | ||
opacityFilter = function( opacity ) { | ||
return '"alpha(opacity=' + opacity + ')"'; |
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.
We are using double quotes in JS so please also use it here. You'll just need to escape the inner double quotes.
Co-authored-by: Michał Gołębiowski-Owczarek <[email protected]>
vars.opacityFilterShadow = opacityFilter( vars.opacityShadow ); | ||
vars.opacityOverlay = opacityFix( vars.opacityOverlay ); | ||
vars.opacityShadow = opacityFix( vars.opacityShadow ); | ||
if ( options.version && semver.lt( options.version, "1.13.0" ) ) { |
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’ve just noticed - don’t nest this all within the else
above, just use else if
. That will help avoid all these indentation changes below as well.
return ( opacity / 100 ).toString().replace( /^0\./, "." ); | ||
}; | ||
opacityFilter = function( opacity ) { | ||
return "alpha(opacity=" + opacity + ")"; |
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.
There are still no quotes wrapping the whole value here. -ms-filter
requires them.
@mcanepa are you interested in finishing this? I figured it was too hard to contribute here: there were some tests but only for a single version and there was no CI that would run it automatically. I added a GitHub Actions workflow in #16 and then added tests for 1.13.2 as well in #18. You should discard changes in fixtures, rebase over the latest |
Does this fix #2190?