Skip to content

Conversation

mpetrovich
Copy link

Known issues:

  1. still vulnerable to positional css selectors: .class:first, :nth-child, etc.

@@ -13,51 +13,127 @@
*/
(function( $, undefined ) {

var clipRegex = /^rect\((-?\d*.?\d*px|-?\d+%|auto),?\s+(-?\d*.?\d*px|-?\d+%|auto),?\s+(-?\d*.?\d*px|-?\d+%|auto),?\s+(-?\d*.?\d*px|-?\d+%|auto)\)$/,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's gotta be a shorter version of this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You prolly want to escape that . since now it acts like a "any character"-capture
which makes this valid: rect(#10px, Mpx, 100!px, 2A0px)

Solution:

(-?\d*\.?\d*px|-?\d+%|auto)

@mikesherov only thing I can think of is adding {4} after the capture group, thus this would only capture one big group instead of 4 afaik =/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jellyfrog How would you repeat the capture group to extract the necessary fields? In JS, only the last capture is kept.

A more maintainable solution might be to first split on delimiters and then extract each clip dimension in an exec loop. I'll test out that approach.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, thereof the

thus this would only capture one big group instead of 4 afaik =/

a while((match = regex.exec(str)) !== null) might look better yes, tho I don't mind the current regex, except for the bug (see my prev. comment)

@mikesherov
Copy link
Member

For now, don't worry about positional selectors being effected by the shadow.

var clipRegex = /^rect\((-?\d*.?\d*px|-?\d+%|auto),?\s+(-?\d*.?\d*px|-?\d+%|auto),?\s+(-?\d*.?\d*px|-?\d+%|auto),?\s+(-?\d*.?\d*px|-?\d+%|auto)\)$/,
parseClip = function( str, outerWidth, outerHeight ) {
var clip,
values = clipRegex.exec( str );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

byte saver:

values = clipRegex.exec( str ) || [ "", 0, outerWidth, outerHeight, 0 ];

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funny, I originally did the same thing but changed it to its current form to prevent the unnecessary ternary and parseInt calls for the most common case where clip is auto. Another case of premature optimization on my part ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-) Speed here takes a back seat to size and maintainability, as @scottgonzalez mentions.

@mikesherov
Copy link
Member

I'm going to create a createPlaceholder method for effects that does the shadow functionality. I'm going to see how it can be used in the other effects.

values[ 3 ] = "auto";
}

clip = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for clip variable. Just return the object.

@mikesherov
Copy link
Member

Closing this for now, @mpetrovich. I'll open a new one so you're not a blocker. Thanks for contributing!

@mikesherov mikesherov closed this Mar 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants