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

Support custom rules #20

Closed
wants to merge 8 commits into from
Closed

Conversation

@olslash
Copy link

@olslash olslash commented May 26, 2016

This gives the ability to add custom linkify-it handlers to individual react-linkify instances through the handlers prop. Additionally, i exposed the linkify-it singleton to enable global customization.

I also cherry-picked the test-suite updates from #19.

Fixes #10

@olslash olslash force-pushed the bottlenose-inc:support-custom-rules branch from 7f11186 to d8b7ac4 May 27, 2016
@olslash
Copy link
Author

@olslash olslash commented May 27, 2016

One issue i'm noticing is that i can't easily merge custom global handlers with instance handlers, because linkify-it doesn't expose them. I could read its internal schemas map (https://github.com/markdown-it/linkify-it/blob/master/index.js#L396) but it's a private property.

@tasti
Copy link
Owner

@tasti tasti commented Jun 3, 2016

@olslash What about instead of having a global instance of LinkifyIt, we expose a way to set global handlers, and when each custom LinkifyIt instance is being defined, it appends the global handlers?

@olslash
Copy link
Author

@olslash olslash commented Jun 3, 2016

That's a good idea.

My only concern is that as we try to support the rest of the LinkifyIt customizations (custom TLDs and the fuzzy* options), we end up duplicating more and more of that library's logic. For example, it would be nice to keep the same chained API that they use to avoid having two sets of docs, but then react-linkify has to mirror their code. It's simple code, but not entirely trivial.

That's why I had the thought of using their private __schemas__ (and __tlds__), but that brings its own problems.

Anyway for now I'll write the custom handler code from the direction you suggested, and we can see how it looks.

edit: i think i can get around duplicating anything, now that I think about it.

* upstream/master:
  Update dist
  Add support for passing wrapping span's className in as prop. Default to Linkify.
  Upgrade babel-jest, jest-cli, and jest Require default Linkify in tests
@olslash
Copy link
Author

@olslash olslash commented Jun 3, 2016

I took a stab at it -- let me know what you think and I'll write docs if it's ok

@olslash olslash force-pushed the bottlenose-inc:support-custom-rules branch from e7667b8 to a861b0a Jun 3, 2016
@olslash olslash force-pushed the bottlenose-inc:support-custom-rules branch from a861b0a to 35512d0 Jun 3, 2016
}

// add instance customizations
(handlers || []).forEach((handler) => {

This comment has been minimized.

@tasti

tasti Jun 6, 2016
Owner

handlers defaults to [], so handlers.forEach(...); should be sufficient.

addCustomHandlers() {
const { handlers } = this.props;

this.linkify = this.linkify || new LinkifyIt();

This comment has been minimized.

@tasti

tasti Jun 6, 2016
Owner

How about we move this.linkify = new LinkifiyIt(); into componentDidMount() so that this.linkify will always be defined when calling addCustomHandlers().


// add instance customizations
(handlers || []).forEach((handler) => {
this.linkify.add(handler.prefix, {

This comment has been minimized.

@tasti

tasti Jun 6, 2016
Owner

What happens to handlers that are removed in the new props? Removed handlers should be set to null to disable them (see https://github.com/markdown-it/linkify-it#addschema-definition).

@tasti
Copy link
Owner

@tasti tasti commented Jun 6, 2016

Looks good for the most part and great job mirroring the linkify-it api.

A concern that I thought of now is that this would make the Linkify component coupled with the linkify-it library. I'm wondering if this is the best way to go especially for cases where some other library has other benefits.

One idea to decouple the two is to have the Linkify component take a getMatches (type: function) prop, which would allow you to freely use any matching implementation you like. For example, if I'd like to use the linkify-it library, I would instantiate and set handlers on my own, and simply pass in this.linkify.match to the Linkify component. As a consequence, this would require you to put the pieces together yourself. What are your thoughts?

@olslash
Copy link
Author

@olslash olslash commented Jun 7, 2016

That definitely leads to a more generic component -- I just wonder if it's at the cost of ease-of-use, and if supporting multiple matching libraries is really an important goal, considering how much linkify-it can be customized.

With the prop approach, I have to manage a linkify-it instance in each place I want to use the component. Also, even if I wanted to use a custom matching library, linkify-it would still be a dependency of the react-linkify so I'm not saving much there.

Possibly a good middle ground is to combine the getMatches prop + the possibility of using a custom global matcher. For my project, I would use a linkify-it instance as my global matcher with my global handlers set. For someone else, it might be some other library.

@tasti tasti mentioned this pull request Aug 13, 2016
@tasti tasti mentioned this pull request Jan 6, 2018
15 of 16 tasks complete
@tasti
Copy link
Owner

@tasti tasti commented Jan 7, 2018

As an effort to make this more generic, you'll have the flexibility to support this in v1.0.0. For example:

const matchDecorator = (text) => {
  // Implement your matching logic here to using your customized instance of LinkifyIt
};

<Linkify matchDecorator={matchDecorator}>
  ...
</Linkify>

matchDecorator lets you override the default linkify-it logic to match URLs however you'd like.

Any feedback is appreciated.

@tasti tasti closed this Jan 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.