Add a title to the link node to support titles in markdown #4161
Conversation
themagickoala
requested review from
zurfyx,
fantactuka,
acywatson,
tylerjbainbridge and
thegreatercurve
as code owners
|
Hi @themagickoala! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
There was a problem hiding this comment.
LGTM, thank you! cc @fantactuka if you want to double check the MD stuff
| // TODO Fix nullish on LinkNode | ||
| if (str != null) { |
There was a problem hiding this comment.
If I'm reading this right, it's guaranteed to be null or string no? So we can do !== null instead and drop the TODO?
There was a problem hiding this comment.
Probably, but this was a copy and paste of target/rel versions, which are also string | null.
There was a problem hiding this comment.
I'm fine with this - we can fix it now or we can merge this and fix them all in a separate PR -all good either way for me.
| importRegExp: /(?:\[([^[]+)\])(?:\(([^()]+)\))/, | ||
| regExp: /(?:\[([^[]+)\])(?:\(([^()]+)\))$/, | ||
| importRegExp: /(?:\[([^[]+)\])(?:\((?:([^()\s]+)(?:\s"([^"]+)"\s*)?)\))/, | ||
| regExp: /(?:\[([^[]+)\])(?:\((?:([^()\s]+)(?:\s"([^"]+)"\s*)?)\))$/, |
There was a problem hiding this comment.
Any chance to check if " was escaped in title?
[Text](https://test.com "Title \" with escaped quote")
There was a problem hiding this comment.
Good catch, I'll try and add a test case for that too, and fix the other unit test that's broken.
There was a problem hiding this comment.
Ok, those should both be sorted now.
|
@zurfyx can this be merged now? |
themagickoala
force-pushed
the
add-title-to-link
branch
from
1f2f807 to
4970c71
Compare
|
@fantactuka @zurfyx I've rebased this to fix the conflict created by my other PR (#4168) so it should be good to merge now I think. |
|
Thank you! Sorry for the delay |
We have a use case for supporting titles in link markdown nodes (see markdown spec). We're currently copying source code and modifying it inside our own project, but that's not really sustainable so here's a PR to add it to Lexical.
Note that this doesn't add support for adding the title via the WYSIWYG editor, but it will support adding it in Markdown and then converting back to rich text.