Enhance ExpensiMark blockquote parsing for Live Markdown Preview #602
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
Skalakid
changed the title
melvin-bot
bot
requested review from
youssef-lr
and removed request for
a team
__tests__/ExpensiMark-HTML-test.js
Outdated
| @@ -668,7 +668,7 @@ test('Test urls with unmatched closing parentheses autolinks correctly', () => { | |||
| testString: 'google.com/(toto))titi)', | |||
| resultString: '<a href="https://google.com/(toto)" target="_blank" rel="noreferrer noopener">google.com/(toto)</a>)titi)', | |||
| }, | |||
|
|
|||
__tests__/ExpensiMark-HTML-test.js
Outdated
| @@ -1491,3 +1490,91 @@ test('Mention', () => { | |||
| testString = '@[email protected]'; | |||
| expect(parser.replace(testString)).toBe('<mention-user>@[email protected]</mention-user>'); | |||
| }); | |||
|
|
|||
| describe('edit mode', () => { | |||
There was a problem hiding this comment.
Even though this is the terminology I've used to explain the task, I would refrain from using "edit mode" here since ExpensiMark is stateless and thus has no context of editing. Let's substitute it with "when should keep whitespace flag is enabled" or something related to it.
__tests__/ExpensiMark-HTML-test.js
Outdated
| expect(parser.replace(quoteTestStartString, {shouldKeepWhitespace: true})).toBe(quoteTestReplacedString); | ||
| }); | ||
|
|
||
| test('quote with space', () => { |
There was a problem hiding this comment.
Nothing important but let's emphasize the fact that here we have exactly one space.
| test('quote with space', () => { | |
| test('quote with single space', () => { |
__tests__/ExpensiMark-HTML-test.js
Outdated
| expect(parser.replace(quoteTestStartString, {shouldKeepWhitespace: true})).toBe(quoteTestReplacedString); | ||
| }); | ||
|
|
||
| test('quote with a lot of spaces', () => { |
There was a problem hiding this comment.
| test('quote with a lot of spaces', () => { | |
| test('quote with multiple spaces', () => { |
__tests__/ExpensiMark-HTML-test.js
Outdated
| expect(parser.replace(quoteTestStartString, {shouldKeepWhitespace: true})).toBe(quoteTestReplacedString); | ||
| }); | ||
|
|
||
| test('mixed blocqoutes', () => { |
There was a problem hiding this comment.
| test('mixed blocqoutes', () => { | |
| test('separate blockqoutes', () => { |
__tests__/ExpensiMark-HTML-test.js
Outdated
| expect(parser.replace(quoteTestStartString, {shouldKeepWhitespace: true})).toBe(quoteTestReplacedString); | ||
| }); | ||
|
|
||
| test('nested quote and heading', () => { |
There was a problem hiding this comment.
Let's group these tests in describe('nested heading in blockquote') to avoid repetition in test names.
__tests__/ExpensiMark-HTML-test.js
Outdated
| }); | ||
|
|
||
| test('nested quote and heading', () => { | ||
| const quoteTestStartString = '># Hello world'; |
__tests__/ExpensiMark-HTML-test.js
Outdated
| expect(parser.replace(quoteTestStartString, {shouldKeepWhitespace: true})).toBe(quoteTestReplacedString); | ||
| }); | ||
|
|
||
| test('nested quote and heading with space between', () => { |
There was a problem hiding this comment.
| test('nested quote and heading with space between', () => { | |
| test('with single spaces', () => { |
__tests__/ExpensiMark-HTML-test.js
Outdated
| expect(parser.replace(quoteTestStartString, {shouldKeepWhitespace: true})).toBe(quoteTestReplacedString); | ||
| }); | ||
|
|
||
| test('nested quote and heading with many spaces after #', () => { |
There was a problem hiding this comment.
| test('nested quote and heading with many spaces after #', () => { | |
| test('with multiple spaces after #', () => { |
lib/ExpensiMark.js
Outdated
| if (shouldKeepWhitespace) { | ||
| return textToProcess.replace(/^# ( *(?! )(?:(?!<pre>|\n|\r\n).)+)/gm, replacement); | ||
| } | ||
| return textToProcess.replace(/^# +(?! )((?:(?!<pre>|\n|\r\n).)+)/gm, replacement); |
There was a problem hiding this comment.
Instead of calling .replace in two places, can we assign regex conditionally, i.e.
| if (shouldKeepWhitespace) { | |
| return textToProcess.replace(/^# ( *(?! )(?:(?!<pre>|\n|\r\n).)+)/gm, replacement); | |
| } | |
| return textToProcess.replace(/^# +(?! )((?:(?!<pre>|\n|\r\n).)+)/gm, replacement); | |
| const regexp = shouldKeepWhitespace | |
| ? /^# ( *(?! )(?:(?!<pre>|\n|\r\n).)+)/gm | |
| : /^# +(?! )((?:(?!<pre>|\n|\r\n).)+)/gm; | |
| return textToProcess.replace(regexp, replacement); |
| ); | ||
| if (shouldKeepWhitespace) { | ||
| return textToProcess.replace(regex, g1 => replacement(g1, shouldKeepWhitespace)); |
There was a problem hiding this comment.
Can we skip calling modifyTextForQuote in this case?
There was a problem hiding this comment.
I'm not sure, after skipping modifyTextForQuote 13 test fails since this method is trimming spaces and newlines in strings. I didn't want to break the existing behavior so I've added a new one but behind the shouldKeepWhitespace flag
There was a problem hiding this comment.
Okay, thanks for explanation.
This PR:
Screen.Recording.2023-11-13.at.12.29.14.mov
Fixed Issues
$ Expensify/App#31178
$ Expensify/App#31179
$ Expensify/App#31182
Tests
To test these changes I've added proper tests connected to all needed behaviors for Live Markdown Preview. To test it by yourself you will need example app.
QA
Same as tests