This repository has been archived by the owner. It is now read-only.

Update /r2/r2/lib/cssfilter.py: minor cleanup and support for CSS3 transforms #368

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@Dakta
Contributor

Dakta commented Mar 12, 2012

Done:

  1. cleanup on some CSS property regexes (replaced \s+ with {w});
  2. add CSS3 transform and transform-origin properties;
  3. add associated translation-value, translation-function, and angle macros.
Update /r2/r2/lib/cssfilter.py: cleanup on CSS property regexes; add …
…CSS3 `transform` and `transform-origin` properties; add associated `translation-value`, `translation-function`, and `angle` macros. -dakta
@chromakode

This comment has been minimized.

Show comment
Hide comment
@chromakode

chromakode Mar 12, 2012

Contributor

Sweeeeeet. Will review ASAP. Thanks!

Contributor

chromakode commented Mar 12, 2012

Sweeeeeet. Will review ASAP. Thanks!

@Dakta

This comment has been minimized.

Show comment
Hide comment
@Dakta

Dakta Mar 12, 2012

Contributor

I tested the REGEXes locally, but don't have a Reddit install to make sure the whole thing works. Once I get a working install, I'll see about updating cssutils. While I'm doing that, I'll also refactor the sortof hacky custom macro/properties setup you've got now, as cssutils supports easy addition of custom profiles. The custom macros and properties are even in the right format already, so I just have to call a function and pass the variables. :)

Happy to help.

Contributor

Dakta commented Mar 12, 2012

I tested the REGEXes locally, but don't have a Reddit install to make sure the whole thing works. Once I get a working install, I'll see about updating cssutils. While I'm doing that, I'll also refactor the sortof hacky custom macro/properties setup you've got now, as cssutils supports easy addition of custom profiles. The custom macros and properties are even in the right format already, so I just have to call a function and pass the variables. :)

Happy to help.

@chromakode

This comment has been minimized.

Show comment
Hide comment
@chromakode

chromakode Mar 12, 2012

Contributor

That would be totally awesome. <3

Let me know our there's anything I can do to help you get rolling!

Contributor

chromakode commented Mar 12, 2012

That would be totally awesome. <3

Let me know our there's anything I can do to help you get rolling!

+
+ # http://www.w3.org/TR/css3-2d-transforms/
+ 'translation-value': r'{length}|{percentage}',
+ 'transform-function': r'matrix({num},{w}{num},{w}{num},{w}{num},{w}{num},{w}{num})|translate({translation-value}(,{w}{translation-value})?)|translateX({translation-value})|translateY({translation-value})|scale({num}(,{w}{num})?)|scaleX({num})|scaleY({num})|rotate({angle})|skewX({angle})|skewY({angle})',

This comment has been minimized.

@chromakode

chromakode Mar 13, 2012

Contributor

Nice! Nitpick: should you allow/consume {w} whitespace before the commas as well?

@chromakode

chromakode Mar 13, 2012

Contributor

Nice! Nitpick: should you allow/consume {w} whitespace before the commas as well?

@@ -88,7 +93,7 @@
'background': r'{bg-gradients}',
'background-image': r'{bg-gradients}',
'background-color': r'{color}',
- 'background-position': r'(({percentage}|{length}){0,3})?\s*(top|center|left)?\s*(left|center|right)?',
+ 'background-position': r'(({percentage}|{length}){0,3})?{w}(top|center|left)?{w}(left|center|right)?',

This comment has been minimized.

@chromakode

chromakode Mar 13, 2012

Contributor

Could you please split cleanup into a separate commit?

@chromakode

chromakode Mar 13, 2012

Contributor

Could you please split cleanup into a separate commit?

@@ -96,6 +101,10 @@
'border-bottom-right-radius': r'{border-radius}',
'border-bottom-left-radius': r'{border-radius}',
'border-top-left-radius': r'{border-radius}',
+
+ # http://www.w3.org/TR/css3-2d-transforms/
+ 'transform': r'none|({transform-function}({w}{transform-function})*)',

This comment has been minimized.

@chromakode

chromakode Mar 13, 2012

Contributor

Are the outer parens (grouping) necessary on the right side of the |?

@chromakode

chromakode Mar 13, 2012

Contributor

Are the outer parens (grouping) necessary on the right side of the |?

This comment has been minimized.

@Dakta

Dakta Mar 13, 2012

Contributor

No. Removed.

@Dakta

Dakta Mar 13, 2012

Contributor

No. Removed.

+
+ # http://www.w3.org/TR/css3-2d-transforms/
+ 'transform': r'none|({transform-function}({w}{transform-function})*)',
+ 'transform-origin': r'(top|bottom)|(({percentage}|{length}|left|center|right)({w}({percentage}|{length}|top|center|bottom))?)|((center|(left|right)({w}({percentage}|{length}))?)&&(center|(top|bottom)({w}({percentage}|{length}))?))',

This comment has been minimized.

@chromakode

chromakode Mar 13, 2012

Contributor

I believe the "&&" in the spec specifies that the two related clauses must be present together; not in the syntax literally. Similar to above, can you remove some of the outer groups?

@chromakode

chromakode Mar 13, 2012

Contributor

I believe the "&&" in the spec specifies that the two related clauses must be present together; not in the syntax literally. Similar to above, can you remove some of the outer groups?

This comment has been minimized.

@Dakta

Dakta Mar 14, 2012

Contributor

Actually, && is how you specify that two regex groups must both apply for it to pass, which is what the spec implies.

@Dakta

Dakta Mar 14, 2012

Contributor

Actually, && is how you specify that two regex groups must both apply for it to pass, which is what the spec implies.

This comment has been minimized.

@chromakode

chromakode Mar 14, 2012

Contributor

Sorry, I'm not sure I follow. I can't find && in the Python regex documentation, and running re.match("(a)&&(b)", "ab") == None. Does it not suffice to include both groups, one after the other? Is the && intended to be in the CSS? Also, it looks like you may need some more {w}s to separate the parameters. For instance, in testing I found that "center&&center" is matched while center && center isn't.

Looking forward to getting this out and seeing what people do with it. :)

@chromakode

chromakode Mar 14, 2012

Contributor

Sorry, I'm not sure I follow. I can't find && in the Python regex documentation, and running re.match("(a)&&(b)", "ab") == None. Does it not suffice to include both groups, one after the other? Is the && intended to be in the CSS? Also, it looks like you may need some more {w}s to separate the parameters. For instance, in testing I found that "center&&center" is matched while center && center isn't.

Looking forward to getting this out and seeing what people do with it. :)

This comment has been minimized.

@Dakta

Dakta Mar 15, 2012

Contributor

I know the double ampersands work in Java regex, and it doesn’t make any sense to me to have consecutive capture groups be anything but consecutive groups. I’m looking for a binary operation here, intersection, where two patters both have to match for it to pass.

I got the reddit VM up and running, so I’ll be testing all my code now. That’ll be good.

On Mar 13, 2012, at 10:02 15PM, Max Goodman wrote:

@@ -96,6 +101,10 @@
'border-bottom-right-radius': r'{border-radius}',
'border-bottom-left-radius': r'{border-radius}',
'border-top-left-radius': r'{border-radius}',

  • http://www.w3.org/TR/css3-2d-transforms/

  • 'transform': r'none|({transform-function}({w}{transform-function})*)',
  • 'transform-origin': r'(top|bottom)|(({percentage}|{length}|left|center|right)({w}({percentage}|{length}|top|center|bottom))?)|((center|(left|right)({w}({percentage}|{length}))?)&&(center|(top|bottom)({w}({percentage}|{length}))?))',

Sorry, I'm not sure I follow. I can't find && in the Python regex documentation, and running re.match("(a)&&(b)", "ab") == None. In case I'm mistaken, including the two groups one after the other will have to match both. Is the && intended to be in the CSS? Also, it looks like you may need some more {w}s to separate the parameters. For instance, in testing I found that "center&&center" is matched while center && center isn't.

Looking forward to getting this out and seeing what people do with it. :)


Reply to this email directly or view it on GitHub:
https://github.com/reddit/reddit/pull/368/files#r555862

@Dakta

Dakta Mar 15, 2012

Contributor

I know the double ampersands work in Java regex, and it doesn’t make any sense to me to have consecutive capture groups be anything but consecutive groups. I’m looking for a binary operation here, intersection, where two patters both have to match for it to pass.

I got the reddit VM up and running, so I’ll be testing all my code now. That’ll be good.

On Mar 13, 2012, at 10:02 15PM, Max Goodman wrote:

@@ -96,6 +101,10 @@
'border-bottom-right-radius': r'{border-radius}',
'border-bottom-left-radius': r'{border-radius}',
'border-top-left-radius': r'{border-radius}',

  • http://www.w3.org/TR/css3-2d-transforms/

  • 'transform': r'none|({transform-function}({w}{transform-function})*)',
  • 'transform-origin': r'(top|bottom)|(({percentage}|{length}|left|center|right)({w}({percentage}|{length}|top|center|bottom))?)|((center|(left|right)({w}({percentage}|{length}))?)&&(center|(top|bottom)({w}({percentage}|{length}))?))',

Sorry, I'm not sure I follow. I can't find && in the Python regex documentation, and running re.match("(a)&&(b)", "ab") == None. In case I'm mistaken, including the two groups one after the other will have to match both. Is the && intended to be in the CSS? Also, it looks like you may need some more {w}s to separate the parameters. For instance, in testing I found that "center&&center" is matched while center && center isn't.

Looking forward to getting this out and seeing what people do with it. :)


Reply to this email directly or view it on GitHub:
https://github.com/reddit/reddit/pull/368/files#r555862

@spladug

This comment has been minimized.

Show comment
Hide comment
@spladug

spladug Jul 15, 2013

Contributor

I'm going to close this because it has been hanging around so long. Please feel free to open a new one if you have an updated version of the patch. Thanks!

Contributor

spladug commented Jul 15, 2013

I'm going to close this because it has been hanging around so long. Please feel free to open a new one if you have an updated version of the patch. Thanks!

@spladug spladug closed this Jul 15, 2013

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.