-
Notifications
You must be signed in to change notification settings - Fork 135
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
Allow codefence regex to accept any characters after first three backticks #788
Conversation
…ktick Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>
cc @akinwale |
@akinwale Just to confirm when sending message for example:
The displayed message is:
The
This is expected behavior right? |
cc @AndrewGable |
cc @akinwale - Want to review? |
I don't think this is expected behaviour. The characters within the codefence should be included instead of being stripped out. |
@akinwale so the expected result would be:
Right? |
Yes, this is correct. |
I will volley here and say that yes, this is the expected behavior for every Markdown variant I have ever come across. If you want your text to appear in your codefence, you need to make sure it's after the line containing the triple backticks. In some variants the area after the backticks is reserved for specifying what flavor of context highlighting you want for you code. |
@deetergp I have made some modifications to the regex; it now functions as shown in this video: Test video:codefence--with-syntax.mp4Currently, it works offline. This is achieved by adding a
This will be converted to: It is currently working offline because the server removes all attributes from the What do you think? Should we proceed with this solution? |
Or maybe we just want to address the original issue by allowing only white spaces after the first three backticks. |
Yeah, I think allowing anything after the three backticks is fine, but expected behavior should be that they are not rendered in the code block. |
@tsa321 Let's not block on that. Right now we don't really support any kind of context highlighting, so there's no real reason for text to be there anyway. |
@akinwale based on the expected results above, my code changes in this PR are fine. |
kindly bump for review @akinwale |
kindly bump for review @akinwale. |
Reviewing this today. |
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.
LGTM.
@tsa321 Looks good, thanks. Will wait for internal review from @deetergp or @AndrewGable and then you can create the corresponding PR in the Expensify App repo. |
🚀Published to npm in v2.0.84 |
Fixed Issues
$ Expensify/App#45088
Tests
I have add unit tests for this issue.
Update the related regex in the expensify-common in node module and send the test messages from composer of the App.
Test example:
The code fence/ code block must be displayed properly.
Verify the displayed message is displayed correctly.
QA
Same as test above
Code fence related regression tests.