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

Highlight Filter Capture Regex #292

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
5 participants
@garethdjames

Added a parameter to support highlighting based on a capturing regular expression

@ProLoser

This comment has been minimized.

Show comment Hide comment
@ProLoser

ProLoser Dec 5, 2012

Member

Okay, why the hell did Travis only run 1 test?

Member

ProLoser commented Dec 5, 2012

Okay, why the hell did Travis only run 1 test?

@ProLoser

View changes

modules/filters/highlight/highlight.js
+ text = text.toString();
+ search = search.toString();
+ var match;
+ if (caseSensitive) {

This comment has been minimized.

Show comment Hide comment
@ProLoser

ProLoser Dec 5, 2012

Member

Perhaps it's more efficient to do:

var scope = 'g';
if (!caseSensitive)
  scope += 'i';
// or
var scope = caseSensitive && 'g' || 'gi';

Rather than duplicate a more complex line of logic.

@ProLoser

ProLoser Dec 5, 2012

Member

Perhaps it's more efficient to do:

var scope = 'g';
if (!caseSensitive)
  scope += 'i';
// or
var scope = caseSensitive && 'g' || 'gi';

Rather than duplicate a more complex line of logic.

@ProLoser

View changes

modules/filters/highlight/highlight.js
+ }
+ var firstPart = text.substr(0, text.indexOf(match[1]));
+ var secondPart = '<span class="ui-match">' + match[1] + '</span>';
+ var lastPart = text.substr(text.indexOf(match[1]) + match[1].length, text.length);

This comment has been minimized.

Show comment Hide comment
@ProLoser

ProLoser Dec 5, 2012

Member

Is it not more efficient to use replace() with 2 parameters instead of this?

@ProLoser

ProLoser Dec 5, 2012

Member

Is it not more efficient to use replace() with 2 parameters instead of this?

@ProLoser

View changes

modules/filters/highlight/highlight.js
+ return text.replace(new RegExp(search, 'gi'), '<span class="ui-match">$&</span>');
+ }
+ } else {
+ return text;

This comment has been minimized.

Show comment Hide comment
@ProLoser

ProLoser Dec 5, 2012

Member

OCD: can cut down the line count by doing:

text = text.split(search).join(etc)
...
return text

Rather than putting else-cases everywhere (in case we add more cases later).

@ProLoser

ProLoser Dec 5, 2012

Member

OCD: can cut down the line count by doing:

text = text.split(search).join(etc)
...
return text

Rather than putting else-cases everywhere (in case we add more cases later).

@NARKOZ

This comment has been minimized.

Show comment Hide comment
@NARKOZ

NARKOZ Dec 5, 2012

@ProLoser you can go to Travis build page and re-run the build under the gear icon.

NARKOZ commented Dec 5, 2012

@ProLoser you can go to Travis build page and re-run the build under the gear icon.

@garethdjames

This comment has been minimized.

Show comment Hide comment
@garethdjames

garethdjames Dec 6, 2012

I have updated the code to reduce the LOC and support the new capture option.
The new code always uses a regex but reduces the LOC and complexity

I have updated the code to reduce the LOC and support the new capture option.
The new code always uses a regex but reduces the LOC and complexity

@garethdjames

This comment has been minimized.

Show comment Hide comment
@garethdjames

garethdjames Dec 6, 2012

Don't merge just yet, there is a change I want to make

Don't merge just yet, there is a change I want to make

@garethdjames

This comment has been minimized.

Show comment Hide comment
@garethdjames

garethdjames Dec 8, 2012

The code before whilst passing all tests was failing in production for a capture highlight at the beginning of the text. I have added new tests for this case and updated the code.

The new code results in 2 regex searches for capture highlight, this could be reduced to 1 regex but would increase the LOC with if/else

The code before whilst passing all tests was failing in production for a capture highlight at the beginning of the text. I have added new tests for this case and updated the code.

The new code results in 2 regex searches for capture highlight, this could be reduced to 1 regex but would increase the LOC with if/else

@ProLoser

This comment has been minimized.

Show comment Hide comment
@ProLoser

ProLoser Feb 1, 2013

Member

@joseym Got another one for you. I just haven't had the time to review the source code and to figure out the new API

Member

ProLoser commented Feb 1, 2013

@joseym Got another one for you. I just haven't had the time to review the source code and to figure out the new API

@ghost ghost assigned joseym Feb 1, 2013

@joseym

This comment has been minimized.

Show comment Hide comment
@joseym

joseym Feb 1, 2013

Contributor

Okay, I'll have a look over the weekend.

Contributor

joseym commented Feb 1, 2013

Okay, I'll have a look over the weekend.

@joseym

This comment has been minimized.

Show comment Hide comment
@joseym

joseym Feb 4, 2013

Contributor

Looks good to me. @garethdjames - from what I gather the capture option forces the match to be a regex, yes?

I played with it in a fiddle: http://jsfiddle.net/joseym/T5uGQ/

I noticed that ([0-9]{3}-[0-9]{3}-[0-9]{4}) will highlight the results regardless of whether or not capture is true, however [0-9]{3}-[0-9]{3}-[0-9]{4} will only highlight the results if capture is false.

I kindof wonder what the point is ... if the regular match accepts regular expressions why have an option to force the match to be a regular expression?

Contributor

joseym commented Feb 4, 2013

Looks good to me. @garethdjames - from what I gather the capture option forces the match to be a regex, yes?

I played with it in a fiddle: http://jsfiddle.net/joseym/T5uGQ/

I noticed that ([0-9]{3}-[0-9]{3}-[0-9]{4}) will highlight the results regardless of whether or not capture is true, however [0-9]{3}-[0-9]{3}-[0-9]{4} will only highlight the results if capture is false.

I kindof wonder what the point is ... if the regular match accepts regular expressions why have an option to force the match to be a regular expression?

@ProLoser

This comment has been minimized.

Show comment Hide comment
@ProLoser

ProLoser Feb 4, 2013

Member

@joseym actually i forgot about that...

Maybe we shouldn't merge this in? Just add some more tests / demos? Perhaps it would be better to simply tweak the case sensitivity option to be like caseInsensitiveOrREGEX etc

Member

ProLoser commented Feb 4, 2013

@joseym actually i forgot about that...

Maybe we shouldn't merge this in? Just add some more tests / demos? Perhaps it would be better to simply tweak the case sensitivity option to be like caseInsensitiveOrREGEX etc

@ProLoser

This comment has been minimized.

Show comment Hide comment
@ProLoser

ProLoser Feb 4, 2013

Member

I'm going to defer this for now to v0.4.1 since it doesn't really introduce any real new functionality and needs to be discussed a bit further.

Member

ProLoser commented Feb 4, 2013

I'm going to defer this for now to v0.4.1 since it doesn't really introduce any real new functionality and needs to be discussed a bit further.

@joseym

This comment has been minimized.

Show comment Hide comment
@joseym

joseym Feb 4, 2013

Contributor

Yeah, with regex ability already in the filter I'm not sure what the capture param gives us.

Contributor

joseym commented Feb 4, 2013

Yeah, with regex ability already in the filter I'm not sure what the capture param gives us.

@garethdjames

This comment has been minimized.

Show comment Hide comment
@garethdjames

garethdjames Feb 5, 2013

The purpose of the parameter was to enable a case where you wanted to enter a regex that would include a capture, i.e. to capture part (or all) of the data.

My use case involves a user writing a regex that will filter (using a () capture) part of the string (in this case a URL). I wanted to show the captured part as highlighted.

e.g.

given the string http://www.somesite.com/index.html?a=b&c=d

and the regex http://(.*)?

the highlight would look like this

http://www.somesite.com/index.html?a=b&c=d

(I'm using 'bold' as the highlight in this email).

If you think this is something worth adding then I can rework it, otherwise I will just add a filter to my own project

Cheers

Gareth

On Monday, 4 February 2013 at 21:53, Josey Morton wrote:

Yeah, with regex ability already in the filter I'm not sure what the capture param gives us.


Reply to this email directly or view it on GitHub (angular-ui#292 (comment)).

The purpose of the parameter was to enable a case where you wanted to enter a regex that would include a capture, i.e. to capture part (or all) of the data.

My use case involves a user writing a regex that will filter (using a () capture) part of the string (in this case a URL). I wanted to show the captured part as highlighted.

e.g.

given the string http://www.somesite.com/index.html?a=b&c=d

and the regex http://(.*)?

the highlight would look like this

http://www.somesite.com/index.html?a=b&c=d

(I'm using 'bold' as the highlight in this email).

If you think this is something worth adding then I can rework it, otherwise I will just add a filter to my own project

Cheers

Gareth

On Monday, 4 February 2013 at 21:53, Josey Morton wrote:

Yeah, with regex ability already in the filter I'm not sure what the capture param gives us.


Reply to this email directly or view it on GitHub (angular-ui#292 (comment)).

@joseym

This comment has been minimized.

Show comment Hide comment
@joseym

joseym Feb 5, 2013

Contributor

Ahh, I see now. I actually think that is rather good, useful, addition now that I understand the point 😄. I updated the fiddle as well.

Contributor

joseym commented Feb 5, 2013

Ahh, I see now. I actually think that is rather good, useful, addition now that I understand the point 😄. I updated the fiddle as well.

@joseym

This comment has been minimized.

Show comment Hide comment
@joseym

joseym Feb 5, 2013

Contributor

I noticed in the fiddle that after the capture is applied the url string is somewhat scattered in the paragraph and some characters that once existed are lost altogether (the ? kicking off the url query is missing after the filter fires)

Contributor

joseym commented Feb 5, 2013

I noticed in the fiddle that after the capture is applied the url string is somewhat scattered in the paragraph and some characters that once existed are lost altogether (the ? kicking off the url query is missing after the filter fires)

@garethdjames

This comment has been minimized.

Show comment Hide comment
@garethdjames

garethdjames Feb 5, 2013

Yes needs a bit more work to get it right

Will update later

On Tuesday, 5 February 2013 at 15:35, Josey Morton wrote:

I noticed in the fiddle that after the capture is applied the url string is somewhat scattered in the paragraph and some characters that once existed are lost altogether (the ? kicking off the url query is missing after the filter fires)


Reply to this email directly or view it on GitHub (angular-ui#292 (comment)).

Yes needs a bit more work to get it right

Will update later

On Tuesday, 5 February 2013 at 15:35, Josey Morton wrote:

I noticed in the fiddle that after the capture is applied the url string is somewhat scattered in the paragraph and some characters that once existed are lost altogether (the ? kicking off the url query is missing after the filter fires)


Reply to this email directly or view it on GitHub (angular-ui#292 (comment)).

@ProLoser

This comment has been minimized.

Show comment Hide comment
@ProLoser

ProLoser May 28, 2013

Member

If this is still a concern can you try adding it to @angular-ui/ui-utils

Member

ProLoser commented May 28, 2013

If this is still a concern can you try adding it to @angular-ui/ui-utils

@ProLoser ProLoser closed this May 28, 2013

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