Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

IE filter property #15

Closed
wants to merge 7 commits into from
Closed

IE filter property #15

wants to merge 7 commits into from

Conversation

mcanepa
Copy link

@mcanepa mcanepa commented Jan 3, 2024

Does this fix #2190?

Copy link
Contributor

@mgol mgol left a 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.

lib/themeroller.js Outdated Show resolved Hide resolved
@mgol
Copy link
Contributor

mgol commented Mar 14, 2024

@mcanepa Hey, do you plan to finish the PR?

@mcanepa
Copy link
Author

mcanepa commented Mar 15, 2024

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

@mgol
Copy link
Contributor

mgol commented Mar 15, 2024

@mcanepa We don't need to handle IE <8 so we just need -ms-filter.

The main issue with the PR right now is you modified an existing block applied to an older jQuery UI version:
https://github.com/jquery/jquery-ui-themeroller/blob/71ccc63fbe0744987743ecf83bcedde460c3165a/lib/themeroller.js#L104-L113
Instead, we need to split this else part:
https://github.com/jquery/jquery-ui-themeroller/blob/71ccc63fbe0744987743ecf83bcedde460c3165a/lib/themeroller.js#L113-L126
into two - one applied to jQuery UI 1.11.x & 1.12.x and then the 1.13+ version will be in the else clause. Contents of the current else clause should be moved to a new else if block dedicated to jQuery UI 1.11.x & 1.12.x.

Does that clarify things?

@mcanepa mcanepa requested a review from mgol March 24, 2024 00:07
lib/themeroller.js Outdated Show resolved Hide resolved
lib/themeroller.js Outdated Show resolved Hide resolved
return ( opacity / 100 ).toString().replace( /^0\./, "." ) + ';-ms-filter: "alpha(opacity=' + opacity + ')"';
};
opacityFilter = function( opacity ) {
return '"alpha(opacity=' + opacity + ')"';
Copy link
Contributor

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.

mcanepa and others added 2 commits March 25, 2024 20:28
Co-authored-by: Michał Gołębiowski-Owczarek <m.goleb@gmail.com>
@mcanepa mcanepa requested a review from mgol March 26, 2024 00:14
lib/themeroller.js Outdated Show resolved Hide resolved
lib/themeroller.js Outdated Show resolved Hide resolved
@mcanepa mcanepa requested a review from mgol March 26, 2024 00:48
lib/themeroller.js Outdated Show resolved Hide resolved
@mcanepa mcanepa requested a review from mgol March 26, 2024 01:41
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" ) ) {
Copy link
Contributor

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 + ")";
Copy link
Contributor

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.

@mgol
Copy link
Contributor

mgol commented Apr 8, 2024

@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 main and you'll see the issues in test results.

@mgol
Copy link
Contributor

mgol commented Apr 20, 2024

@mcanepa thanks for your contribution but I want to move forward with this so I submitted PR #19 instead.

@mgol mgol closed this Apr 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
2 participants