Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Feature: replace text handler #12

Closed

Conversation

badsyntax
Copy link
Contributor

This changes introduces a handler that can be used to replace the matched text with new text (instead of wrapping the matched text in a specified element).

Added example code and description for using the replaceTextHandler function.
@ghost ghost assigned padolsey Sep 16, 2013
@padolsey
Copy link
Owner

Thanks for this Richard -- nice feature! I'll take a look. This can probably go into 0.4.0 (see #13).

@badsyntax
Copy link
Contributor Author

OK cool, I agree with the API changes. I'll close this one then and wait for the new api.

@badsyntax badsyntax closed this Sep 16, 2013
@ethantw
Copy link
Contributor

ethantw commented Sep 17, 2013

Shall we pass the match(es) into the handler so that we can make use of it/them? More like the native .replace() method where we can rearrange the order, modify the matches, or insert text in between with the callback function.

Presenting an example with the current API(0.3.0) below,

findAndReplaceDOMText( 
    /([Miranda|James|David])\s(Hart)/ig,
    document.getElementById('foo'),
    findAndReplaceDOMText.replaceTextHandler(function(matches) {
        var given = matches[0],
             fam = matches[1],
             short_form = fam + ' ' + given.split('')[0] + '. ';

        return short_form;
    }),
    2 // With a function in the handler, ignores the captureGroup
);

@padolsey
Copy link
Owner

I love the idea of being able to supply either a string or a replacement function. I feel like all this replacement stuff needs to be consolidated a bit, along with the suggested API change in #13.

In the meantime I have a question:

What is the expected behaviour when replacing just the text if the text you're searching for crosses node boundaries? In @badsyntax's code, the replacement will work like so (AFAICT):

(replace "foo" with "baz123")
Before:
f<em>o</em><b>o</b>
After:
b<em>a</em><b>z123</b>

I.e. the captured search is substituted with the replacement from left-to-right with remainder characters placed in the last portion of the search (z123). Is this the best/most-intuitive way? I honestly can't think of another way -- other than fully replacing the search, including any intermediary elements, with the replacement.

I'm wondering if this feature is too complex to wrap up in a simple API? Will devs want finer control of how the replacement occurs? (I guess if they do they'll just pass a function, right?)

re-opening since discussion is happening

@padolsey padolsey reopened this Sep 18, 2013
@badsyntax
Copy link
Contributor Author

Some quick thoughts:

  • I initially proposed this 'add-on' handler due the API lacking in this feature, and IMO this library should provide this feature out the box (because it can, because this would be useful, and because the name of the library suggests it does).
  • I feel the implementation of this handler is hacky, and I feel this feature should belong in the core code and not in an additional handler. (I agree that this replacement stuff needs to be consolidated.)
  • Regarding your question, yes I'm not sure the best approach about replacing words that cross nodes. My suggestion for dealing with this is the only way I can think of! I personally think it's fine.
  • "Will devs want finer control of how the replacement occurs.." - i agree they can just use a custom replacement handler.
  • Personally I want this library to provide me with a simple API that I can use to 1) wrap a matched term with wrapper elements (as it does now) and 2) replace a matched term with new text.

@padolsey
Copy link
Owner

Regarding new things in the apiCleanup branch which I just pushed:

You can now replace plain text like so:

findAndReplaceDOMText(nodeToSearchIn, {
  find: /something/,
  replace: 'something else'
});

And wrapping a node, as before, would be done like so:

findAndReplaceDOMText(nodeToSearchIn, {
  find: /something/,
  wrap: 'em'
});

And of course there is still finer control by passing a custom function:

findAndReplaceDOMText(nodeToSearchIn, {
  find: /something/,
  replace: function() {
    // Do what you want
    // Return a node or string
  }
});

Regarding your question, yes I'm not sure the best approach about replacing words that cross nodes. My suggestion for dealing with this is the only way I can think of! I personally think it's fine.

The new API deals with this by having, atm, two modes, 'retain' and 'first'. With the former example:

(replace "foo" with "baz123")

Before:
f<em>o</em><b>o</b>

After (Using RETAIN):
b<em>a</em><b>z123</b>

After (Using FIRST):
baz123<em></em><b></b>

The retain mode is the default. I'm still wondering if having a first mode is even needed -- or if I've done it the best way -- as you can see, currently, it leaves the empty element nodes in-tact and crushes the entire replacement into the very first portion's node.

@padolsey
Copy link
Owner

padolsey commented Oct 6, 2013

0.4.0 is out -- and it includes the ability to replace with text or node(s).

@padolsey padolsey closed this Oct 6, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants