Skip to content
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

update inlineCodeBlock replace regex #607

Merged

Conversation

akamefi202
Copy link
Contributor

This PR updated inlineCodeBlock replace regex to preventing existence of code fence(<pre> tag) inside inline code(<code> tag).
So if <pre> tag already exists inside backticks(&#x60;), it blocks replacement of inlineCodeBlock.

Before update:

`(```test```)` => <code>(<pre>test</pre>)</code>

After update:

`(```test```)` => &#x60;(<pre>test</pre>)&#x60;

Fixed Issues

$ GH_LINK

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
    I added "Test code fence within inline code" test case for this.
  2. What tests did you perform that validates your changed worked?
    I ran "npm run test" and confirmed all tests passed successfully.

QA

  1. What does QA need to do to validate your changes?
    Open any chat.
    Write "(```test```)" in the composer and send the message.
    The message is shown immediately in expected format.
`
test
`
  1. What areas to they need to test for regressions?
    Run "npm run test".

@akamefi202 akamefi202 requested a review from a team as a code owner November 21, 2023 11:07
@melvin-bot melvin-bot bot requested review from thienlnam and removed request for a team November 21, 2023 11:07
@MonilBhavsar MonilBhavsar self-requested a review November 21, 2023 11:14
@MonilBhavsar
Copy link
Contributor

It will be great if it is tested by linking this lib with App repo

@akamefi202
Copy link
Contributor Author

@MonilBhavsar I tested it and attached the record video.

@thienlnam
Copy link
Contributor

In the future, please link the issue with these steps so that it assigns the correct people to review

1. Please postfix `$` with a URL link to the GitHub issue this Pull Request is fixing. For example, `$ https://github.com/Expensify/App/issues/<issueID>`.
2. Please postfix  `PROPOSAL:` with a URL link to your GitHub comment, which contains the approved proposal (i.e. the proposal that was approved by Expensify).  For example, `PROPOSAL: https://github.com/Expensify/App/issues/<issueID>#issuecomment-1369752925`

@thienlnam thienlnam removed their request for review November 21, 2023 21:21
Copy link
Contributor

@MonilBhavsar MonilBhavsar left a comment

Choose a reason for hiding this comment

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

I tested it and attached the record video.

Sorry, could not find it

lib/ExpensiMark.js Show resolved Hide resolved
__tests__/ExpensiMark-HTML-test.js Outdated Show resolved Hide resolved
@akamefi202
Copy link
Contributor Author

@MonilBhavsar I attached the video and resolved problems. Please review again.

Screencast.from.2023.11.21.22.13.30.webm

@MonilBhavsar
Copy link
Contributor

Hmm, I see you just removed those cases. Perhaps we might want to make it work as expected by updating our regex

@akamefi202
Copy link
Contributor Author

akamefi202 commented Nov 22, 2023

@MonilBhavsar You can see that issue on staging server right now and so it's not because of regex update of this PR.
And I think that it is different issue and we should define expected behavior first through discussion.
Same with Slack or not.

I did simple investigation and it is because of space character before backtick.
In slack, inline code block only allows space character before first backtick.
But in our app, inline code block allows any character before first backtick.

bandicam.2023-11-22.09-39-54-719.mp4

@MonilBhavsar
Copy link
Contributor

MonilBhavsar commented Nov 27, 2023

I think you're right. Slack does not consider a code block if it is proceeded by a character. It works in github though.
I feel like for the case mentioned in the issue, we're following slack's markdown which is very different from github or Expensify.

I think the tests you added here were correct but our markdown didn't parse them as expected. Looks like currently in expensify app, when we enter "testspaceblock", it parses it correctly as it was expected in your automated test, but it doesn't seem to work with updated regex?

Seems like we're breaking lots of cases for supporting an edge case.

@akamefi202
Copy link
Contributor Author

@MonilBhavsar Could you please tell me more details? I couldn't understand what you mean.
I tried to test with below message and it was parsed correctly by updated regex.

`test`space```block``` => <code>test</code>space<pre>block</pre>

Could you please let me know what is wrong?

@ArekChr
Copy link

ArekChr commented Nov 28, 2023

@MonilBhavsar, should we create a pull request in NewDot, using a commit hash that points to these changes? This will allow us to test this issue directly within the app and complete the checklist if needed.

@akamefi202 akamefi202 force-pushed the code-block-within-inline-code branch from 92337a1 to 939ccb9 Compare November 30, 2023 22:03
@akamefi202
Copy link
Contributor Author

@MonilBhavsar Can we merge this PR if there isn't other problem? So we can proceed to next stage.

@MonilBhavsar
Copy link
Contributor

should we create a pull request in NewDot, using a commit hash that points to these changes? This will allow us to test this issue directly within the app and complete the checklist if needed.

We can do it locally, right?

@MonilBhavsar
Copy link
Contributor

@MonilBhavsar Could you please tell me more details? I couldn't understand what you mean. I tried to test with below message and it was parsed correctly by updated regex.

`test`space```block``` => <code>test</code>space<pre>block</pre>

Could you please let me know what is wrong?

Yes, that's right and it is how it should be expected. But, if you paste those strings in slack, it is not rendered as expected. I mean to say for the string in the issue - "(```test```)", we decided to update our markdown to match as that of slack, but their parsing is different from ours. So should we match md parsing of github?

@akamefi202
Copy link
Contributor Author

@MonilBhavsar
Our code formatting is different with both Github's and Slack's. It's NewDot's own formatting.
For example, Github doesn't allow code block in one line and Slack doesn't allow inline code without space before it.

In this issue, I suggested to update regex to fix this error and it showed same result with Slack.

App displays empty message on sending the message in specific format for some time and using same format in edit displays empty text until report is revisited

I think we should refer to both of them to get the best UX. Instead of trying to match with one of them.

@ArekChr
Copy link

ArekChr commented Dec 5, 2023

@MonilBhavsar Do we have any assumptions for the code block when it was designed so that we can refer to it?

@ArekChr
Copy link

ArekChr commented Dec 5, 2023

After further research and testing, I have not found a more suitable regex than the one @akamefi202 provided

@MonilBhavsar
Copy link
Contributor

Our code formatting is different with both Github's and Slack's. It's NewDot's own formatting.
App displays empty message on sending the message in specific format for some time and using same format in edit displays empty text until report is revisited
I think we should refer to both of them to get the best UX. Instead of trying to match with one of them.

Yes, I agree. But for this specific issue or code block, we referred to slack's way of parsing markdown and that is very different, I think.

@MonilBhavsar
Copy link
Contributor

Do we have any assumptions for the code block when it was designed so that we can refer to it?

I don't think so. cc @thienlnam if you have any idea, since I believe you are working with live markdown feature so you might know

@MonilBhavsar
Copy link
Contributor

I have not found a more suitable regex than the one @akamefi202 provided

👍 @akamefi202 can we please re add those automated tests that you removed, and perhaps some more?
We can move forward then.

@akamefi202
Copy link
Contributor Author

@MonilBhavsar I added them and please review it.

@thienlnam
Copy link
Contributor

Do we have any assumptions for the code block when it was designed so that we can refer to it?

I don't think so. cc @thienlnam if you have any idea, since I believe you are working with live markdown feature so you might know

Ideally it's just a subset of actual markdown. Though when we were building it out we started with the most straightforward implementation. I'm just guessing this is a bug we didn't intentionally add

@MonilBhavsar
Copy link
Contributor

Ideally it's just a subset of actual markdown. Though when we were building it out we started with the most straightforward implementation. I'm just guessing this is a bug we didn't intentionally add

👍 Right. This is a bug, but the markdown is bit complicated, so not sure how we would parse. So just wanted to get the views. Looks like we're all good here.

@MonilBhavsar
Copy link
Contributor

@akamefi202 this commit is unsigned b896579
And merging is blocked. Could you please sign that commit

@MonilBhavsar
Copy link
Contributor

You can follow these steps to sign that commit

  1. Check out a new branch to test on in case something bad happens

  2. Find the commit before the one you want to change

  3. git rebase -i --preserve-merges <sha>

  4. Change pick to edit for the commit you want to sign

  5. git commit -S --amend --no-edit when it prompts you

  6. git rebase --continue

  7. Push your new branch to github, confirm that it is fixed

  8. Optional: Repeat steps 3-6 on your actual branch and force push. Otherwise just close the old PR and create a new one with the new test branch

@akamefi202 akamefi202 force-pushed the code-block-within-inline-code branch 4 times, most recently from 5b9f396 to d6895b5 Compare December 7, 2023 09:47
Signed-off-by: akamefi202 <akamefi202@gmail.com>
Signed-off-by: akamefi202 <akamefi202@gmail.com>
Signed-off-by: akamefi202 <akamefi202@gmail.com>
Signed-off-by: akamefi202 <akamefi202@gmail.com>
Signed-off-by: akamefi202 <akamefi202@gmail.com>
@akamefi202 akamefi202 force-pushed the code-block-within-inline-code branch from d6895b5 to 4e2404c Compare December 7, 2023 09:48
@akamefi202
Copy link
Contributor Author

@MonilBhavsar I signed commits again and please approve & merge the PR.

@MonilBhavsar
Copy link
Contributor

Thanks!

@MonilBhavsar MonilBhavsar merged commit 927c840 into Expensify:main Dec 7, 2023
5 checks passed
@akamefi202 akamefi202 deleted the code-block-within-inline-code branch December 7, 2023 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants