-
Notifications
You must be signed in to change notification settings - Fork 136
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
fix: allow to add link with code block inside alias text part #558
fix: allow to add link with code block inside alias text part #558
Conversation
Ugh, I tried to assign PullerBear again to fix the assignment but i think I will have to assign manually the CME and C+ |
@hayata-suenaga So the issue wasn't correctly linked in the PR so it failed to assign you, that means you won't get the extra contributions. I could either review it as well or unassign myself, but then, if I review it, I will get the contributions that you should be getting (it's only one though) so up to you. |
I will test it in the morning. It's already EOD for me. Sorry for the delay on this one. |
@pecanoro I'll keep myself assigned I don't care about contribution points so much so please review the PR. I'm not familiar with |
Reviewer Checklist
Screenshots/Videos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests well!
@@ -457,7 +457,7 @@ export default class ExpensiMark { | |||
} | |||
replacedText = replacedText.concat(textToCheck.substr(startIndex, (match.index - startIndex))); | |||
|
|||
if (abort) { | |||
if (abort || match[1].includes('<pre>')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does match[1]
contains the strings captured by regex of capture group?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can we be sure that
should be captured in the first capturing group?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hayata-suenaga That's an interesting question. The order of regex capturing group is determined by the regex expression, from left to right and from outer to inner. Eg, for regex
/(abc(d(e)))(fg)(hi)/gm
See also
expensify-common/lib/ExpensiMark.js
Line 6 in d7e4249
const MARKDOWN_LINK_REGEX = new RegExp(`\\[([^\\][]*(?:\\[[^\\][]*][^\\][]*)*)]\\(${MARKDOWN_URL_REGEX}\\)(?![^<]*(<\\/pre>|<\\/code>))`, 'gi'); |
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Capturing_group
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for the detailed explanation @eh2077 that makes sense 👍
I think the real problem is that So either we should parse it as an inline code block or not parse it. IMO, the code block syntax should be.
There is another Expensify/App#23034 issue that is related inline syntax of the code block. I don't know why we allowed it to be parsed in an inline context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and working as expected!
Any thoughts on my comment #558 (comment) ? |
@parasharrajat Thanks for your comment. I think I misunderstood your point. You suggested to parse inline three backtick For example, markdown |
@parasharrajat This PR shouldn’t conflict with the direction to fix the issue you mentioned because the alias text part of link supports multiple line text, like
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
For example, markdown Good point @eh2077. In that case, we shouldn't allow pre formatting with inline syntax.
|
Fixed Issues
$ Expensify/App#22492
Proposal: Expensify/App#22492 (comment)
Tests
code
is displayed as code block and linkgoogle.com
is displayed as auto link.Demo video
Screen.Recording.2023-07-14.at.5.47.33.PM.mov
QA
Same as test