-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
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 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. |
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? |
@nikgraf Several things:
|
This is soooo good! Thanks @adrianmc |
@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. |
Awesome! Let me see what else I can help out with. |
Closes this issue: #174