Skip to content

feat: Extend support for link target and rel to lexical-react’s AutoLinkPlugin#3209

Merged
acywatson merged 7 commits intofacebook:mainfrom
acusti:feat/link-node-with-target-rel-autolinkplugin
Nov 7, 2022
Merged

feat: Extend support for link target and rel to lexical-react’s AutoLinkPlugin#3209
acywatson merged 7 commits intofacebook:mainfrom
acusti:feat/link-node-with-target-rel-autolinkplugin

Conversation

@acusti
Copy link
Contributor

@acusti acusti commented Oct 18, 2022

this is a follow-up to #2687 (which resolved issue #2671) that extends support for setting link target and rel to the AutoLinkPlugin component via its matchers prop. it adds optional rel and target fields to the LinkMatcherResult type and forwards those along to $createAutoLinkNode, or uses linkNode.setRel / linkNode.setTarget in handleLinkEdit if they have changed.

i also updated the lexical-react docs plugins page’s AutoLinkPlugin section to adopt the default protocol behavior introduced in #2654 and added commented-out example usage of the new LinkMatcher rel and target fields (preview). i wasn’t sure how to properly document it, so if there is a better way (e.g. just a single comment at the beginning or ending of the LinkMatcher object mentioning the optional fields or something else entirely), please let me know.

i didn’t find an open issue for this, though there is a question about it in the original PR thread: #2687 (comment). i’d be happy to open one if that would be helpful.

@vercel
Copy link

vercel bot commented Oct 18, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview Oct 25, 2022 at 8:46PM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview Oct 25, 2022 at 8:46PM (UTC)

Copy link
Contributor

@acywatson acywatson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was originally not sure about making this part of matcher result, but it makes sense because you might want different values here for different matcher types.

Maybe it makes more sense to add one property to the LinkMatcherResult that's a bag of attributes that can be added to the link:

LinkMatcherResult = {
...
attributes: {
rel?: ?string
target?: ?string
}
}

@acusti
Copy link
Contributor Author

acusti commented Oct 20, 2022

@acywatson that would match the API of $createAutoLinkNode; i’m good with that change. should i implement it?

I was originally not sure about making this part of matcher result, but it makes sense because you might want different values here for different matcher types.

in my case, i am using a lexical editor for user generated content, so for this instance of the editor, i’m planning to set target: 'ugc' on all autolinked links (but not mailto links).

@fantactuka
Copy link
Collaborator

fantactuka commented Oct 24, 2022

$createAutoLinkNode accepts attributes as second argument, maybe exposing a type like LinkAttributes and then returning attributes object from matcher with that type (as Acy suggested above) would simplify extending it in future?

$createAutoLinkNode and toggleLink were missing latest changes to link attributes (rel & target) support
@acusti
Copy link
Contributor Author

acusti commented Oct 25, 2022

@fantactuka i added an exported LinkAttributes type in packages/lexical-link/src/index.ts, and i’m importing it into packages/lexical-react/src/LexicalAutoLinkPlugin.ts and using it for a new optional attributes key in the LinkMatcherResult type. does that look like what you were hoping for? (cc: @acywatson)

Copy link
Contributor

@acywatson acywatson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks right to me.

@acusti
Copy link
Contributor Author

acusti commented Nov 4, 2022

i’m happy to rebase this against latest main or do anything else that would be useful for making this PR mergeable, just let me know if there’s anything like that i can do. also, i mentioned this when i opened the PR, but just to be thorough: i wanted to add something to the docs to reference the new optional attributes, so i put a commented-out line in the AutoLinkPlugin usage example: https://github.com/facebook/lexical/pull/3209/files#diff-d59fe907ca78808fad44ab494f4014c15040d7e97abd9270edab884936115bbfR136

i didn’t come across any other references to optional attributes like that in lexical-react’s plugins doc, so i just went for the single-line comment, but if there is a different way you all would prefer, please let me know. or if i should just remove that line from the docs and allow it to be something people discover via typescript types or browsing through source code, that’s also fine with me.

@acywatson acywatson merged commit eba0198 into facebook:main Nov 7, 2022
@acusti acusti deleted the feat/link-node-with-target-rel-autolinkplugin branch November 7, 2022 17:30
thegreatercurve pushed a commit that referenced this pull request Nov 25, 2022
…inkPlugin (#3209)

* fix: Remove redundant conditional check

* Fix out-of-date LexicalLink flow types

$createAutoLinkNode and toggleLink were missing latest changes to link attributes (rel & target) support

* Add and use LinkAttributes type in lexical-link

* feat: Support link target + rel in AutoLinkPlugin

* docs: Cleanup trailing commas in react/plugins.md

* docs: Add default protocol in AutoLinkPlugin example

* docs: Add example usage of LinkMatcher rel/target
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants