Skip to content

Adding ability to set prefix and target for Linkify plugin #175

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

Merged
merged 14 commits into from
Apr 3, 2016

Conversation

adrianmcli
Copy link
Member

Closes this issue: #174

@nikgraf
Copy link
Member

nikgraf commented Apr 3, 2016

While we use babel to compile the code it is shipped to npm as ES5 code. People might not use babel-polyfills and as a library creator you don't want to prescribe them to it. Can you change it to use indexOf? Sorry, didn't see it before.

One more thing that is not necessary, but would make my life a lot easier is to add the new params to the docs here and even extend the CustomLinkifyEditor to use target blank (it's a great example): https://github.com/draft-js-plugins/draft-js-plugins/tree/master/docs/client/components/pages/Linkify

Updating the Changelog in the plugin would be nice as well 😄

Also not necessary, but would be really cool is adding tests for it. That said the test coverage is pretty low except for the plugin editor. I'm aiming to improve it. In any way, I will merge it once the indexOf is change. Let me know if you want to do more.

@adrianmcli
Copy link
Member Author

No worries, great suggestions. I'll get on those changes. I'll probably have it done in a day or so.

As for tests, I'm still pretty green to testing. Do you have any recommendations for how I should approach this? Should I just follow the same pattern in the existing tests file for the Link component?

@adrianmcli
Copy link
Member Author

@nikgraf Several things:

  1. Use of IndexOf() or startsWith() is no longer necessary.

    I've replaced the prefix functionality with Linkify's own intelligent url mode. Previously, if we had google.com and ftp://google.com we would get hrefs like http://google.com and http://ftp://google.com respectively.

    That's obviously bad, but by using Linkify's own methods, the result is now what the user expects (http://google.com and ftp://google.com).

    I've written tests for this, you can take a look. I also don't believe this requires specific documentation since it is the natural behaviour of the Linkify It library (https://github.com/markdown-it/linkify-it).

  2. I have added the _blank example to CustomLinkifyEditor as you have requested. Please take a look.

  3. I have updated the CHANGELOG and Docs as requested.

  4. I added a bunch of tests that I think show the benefits of using the Linkify module's prefix functionality. Take a look to see if you think they are sufficient.

@nikgraf
Copy link
Member

nikgraf commented Apr 3, 2016

This is soooo good! Thanks @adrianmc

@nikgraf nikgraf merged commit 140d013 into draft-js-plugins:master Apr 3, 2016
@nikgraf
Copy link
Member

nikgraf commented Apr 3, 2016

@adrianmc you changes are live on https://www.draft-js-plugins.com

I'm planning on cutting 1.0.0 of the linkify plugin mid/end this week.

@adrianmcli
Copy link
Member Author

Awesome! Let me see what else I can help out with.

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.

2 participants