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.

Sign up for GitHub

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

Add h1 markdown #468

Merged
merged 10 commits into from
Oct 12, 2022
Merged

Add h1 markdown #468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
bf8074e
Add markdown to html rule.
cristipaval Oct 10, 2022
929abd2
Add html to markdown rule.
cristipaval Oct 10, 2022
3ea9691
Add tests for h1 to heading1
cristipaval Oct 10, 2022
29fb855
Remove generated markdown for h1 tags.
cristipaval Oct 10, 2022
56af2fe
Replace h1 with h2 in tests for generated markdown
cristipaval Oct 10, 2022
58f5320
Handle white spaces before and after h1 tags when html to markdown.
cristipaval Oct 10, 2022
650f1ac
Fix JS styling
cristipaval Oct 10, 2022
a1e6aa9
require at least one character after # to avoid empty h1 tags
cristipaval Oct 12, 2022
1f0a776
Split tests.
cristipaval Oct 12, 2022
6cd14a4
Split tests for html to h1 markdown conversion.
cristipaval Oct 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 46 additions & 2 deletions __tests__/ExpensiMark-Markdown-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ test('Test strikethrough HTML replacement', () => {
});

test('Test Mixed HTML strings', () => {
const rawHTMLTestStartString = '<em>This is</em> a <strong>test</strong>. None of <h1>these strings</h1> should display <del>as</del> <div>HTML</div>.';
const rawHTMLTestStartString = '<em>This is</em> a <strong>test</strong>. None of <h2>these strings</h2> should display <del>as</del> <div>HTML</div>.';
const rawHTMLTestReplacedString = '_This is_ a *test*. None of \nthese strings\n should display ~as~ \nHTML\n.';
expect(parser.htmlToMarkdown(rawHTMLTestStartString)).toBe(rawHTMLTestReplacedString);
});
Expand Down Expand Up @@ -412,7 +412,7 @@ test('map div with bold and italics', () => {
});

test('map div with mixed html strings', () => {
const testString = '<div><em>This is</em> a <strong>test</strong>. None of <h1>these strings</h1> should display <del>as</del><div>HTML</div><div></div><em>line 3</em></div>';
const testString = '<div><em>This is</em> a <strong>test</strong>. None of <h2>these strings</h2> should display <del>as</del><div>HTML</div><div></div><em>line 3</em></div>';
const resultString = '_This is_ a *test*. None of \nthese strings\n should display ~as~\nHTML\n_line 3_';
expect(parser.htmlToMarkdown(testString)).toBe(resultString);
});
Expand Down Expand Up @@ -452,3 +452,47 @@ test('map real message with quotes', () => {
const resultString = '\n> hi\n\n\n> hi\n\n';
expect(parser.htmlToMarkdown(testString)).toBe(resultString);
});

test('Test heading1 markdown replacement', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should split these tests up. For example this one is testing

  • normal # Heading case
  • cases with multiple spaces (# Heading)
  • cases with none (#Heading).

You can split this into 3 small and simple tests that'll be easier to read. Also this way there's no need for the long message in the test string that explains everything 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I like your suggestion 👍

const testString = '# This is a heading1 because starts with # followed by a space\n';
const resultString = '<h1>This is a heading1 because starts with # followed by a space</h1><br />';
expect(parser.replace(testString)).toBe(resultString);
});

test('Test heading1 markdown replacement when # is followed by multiple spaces', () => {
const testString = '# This is also a heading1 because starts with # followed by a space\n';
const resultString = '<h1>This is also a heading1 because starts with # followed by a space</h1><br />';
expect(parser.replace(testString)).toBe(resultString);
});

test('Test heading1 markdown when # is not followed by a space', () => {
const testString = '#This is not a heading1 because starts with a # but has no space after it\n';
const resultString = '#This is not a heading1 because starts with a # but has no space after it<br />';
expect(parser.replace(testString)).toBe(resultString);
});

test('Test heading1 markdown when # is in the middle of the line', () => {
const testString = 'This is not # a heading1\n';
const resultString = 'This is not # a heading1<br />';
expect(parser.replace(testString)).toBe(resultString);
});

test('Test html string to heading1 markdown', () => {
const testString = '<h1>This is a heading1</h1><br />';
const resultString = '\n# This is a heading1\n';
expect(parser.htmlToMarkdown(testString)).toBe(resultString);
});

test('Test html to heading1 markdown when there are other tags inside h1 tag', () => {
const testString = '<h1>This is a <strong>heading1</strong></h1>';
const resultString = '\n# This is a *heading1*\n';
expect(parser.htmlToMarkdown(testString)).toBe(resultString);
});

test('Test html to heading1 markdown when h1 tags are in the middle of the line', () => {
const testString = 'this line has a <h1>heading1</h1> in the middle of the line';
const resultString = 'this line has a\n'
+ '# heading1\n'
+ 'in the middle of the line';
expect(parser.htmlToMarkdown(testString)).toBe(resultString);
});
13 changes: 11 additions & 2 deletions lib/ExpensiMark.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,11 @@ export default class ExpensiMark {
},
replacement: g1 => `<blockquote>${g1}</blockquote>`,
},
{
name: 'heading1',
regex: /^#(?!#) +(.+)$/gm,
replacement: '<h1>$1</h1>',
},
Comment on lines +165 to +169
Copy link
Contributor

Choose a reason for hiding this comment

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

This rule was misplaced. It should have been placed before quote rule, because the quote rule consumes the newlines chars i.e. \n. This made such markdown fail > a\n# b. (Coming from Expensify/App#21846)

{
name: 'newline',
regex: /\n/g,
Expand Down Expand Up @@ -223,6 +228,11 @@ export default class ExpensiMark {
regex: /<br(?:"[^"]*"|'[^']*'|[^'"><])*>\n?/gi,
replacement: '\n',
},
{
name: 'heading1',
regex: /\s*<(h1)(?:"[^"]*"|'[^']*'|[^'">])*>(.*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))\s*/gi,
Copy link
Contributor

Choose a reason for hiding this comment

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

👋
This has caused a regression in Expensify/App#17998.
We were removing all of the new line breaks before <h1> tag, (\s), but should remove whitespaces ([^\S\r\n]).
More details here

replacement: '\n# $2\n',
},
{
name: 'italic',
regex: /<(em|i)(?:"[^"]*"|'[^']*'|[^'">])*>(.*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
Expand Down Expand Up @@ -465,8 +475,7 @@ export default class ExpensiMark {
}
generatedMarkdown = generatedMarkdown.replace(rule.regex, rule.replacement);
});
generatedMarkdown = generatedMarkdown
.replace(/<div.*?>|<\/div>|<comment.*?>|\n<\/comment>|<\/comment>|<h1>|<\/h1>|<h2>|<\/h2>|<h3>|<\/h3>|<h4>|<\/h4>|<h5>|<\/h5>|<h6>|<\/h6>/gm, '[block]');
generatedMarkdown = generatedMarkdown.replace(/<div.*?>|<\/div>|<comment.*?>|\n<\/comment>|<\/comment>|<h2>|<\/h2>|<h3>|<\/h3>|<h4>|<\/h4>|<h5>|<\/h5>|<h6>|<\/h6>/gm, '[block]');
return Str.htmlDecode(this.replaceBlockWithNewLine(Str.stripHTML(generatedMarkdown)));
}

Expand Down