Add h1 markdown #468

cristipaval · 2022-10-10T22:06:36Z

Part of this DesignDoc.
Adds support for heading markdown. A line which starts with # and a space (# ) is wrapped with <h1></h1> tag.

Fixed Issues

https://github.com/Expensify/Expensify/issues/225615

Tests

What unit/integration tests cover your change? What autoQA tests cover your change?

there are 2 new automated tests added which checks that markdown is replaced with html tags and vice versa.

What tests did you perform that validates your changes worked?

Link the expensify-common local repository by following these steps
Run the app
Open any chat and send a text message containing lines starting with a # and a space and then text.
Check that the lines from the previous step are rendered as headings
copy the message to the clipboard and paste it into the composer (input text area)
check that the message is parsed correctly and that the headings are pasted as lines starting with # and space

QA

What does QA need to do to validate your changes?

Run the app
Open any chat and send a text message containing lines starting with a # and a space and then text.
Check that the lines from the previous step are rendered as headings
copy the message to the clipboard and paste it into the composer (input text area)
check that the message is parsed correctly and that the headings are pasted as lines starting with # and space

What areas to they need to test for regressions?

Sending messages with all the markdowns should be tested to check that the markdown rules are applied correctly in different markdown combinations
Copy-Paste the above messages to check that they are correctly converted back from html to markdown

github-actions · 2022-10-10T22:06:50Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

cristipaval · 2022-10-10T22:10:59Z

I have read the CLA Document and I hereby sign the CLA

lib/ExpensiMark.js

arosiclair · 2022-10-11T16:24:37Z

__tests__/ExpensiMark-Markdown-test.js

@@ -452,3 +452,35 @@ 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', () => {


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 😅

yeah I like your suggestion 👍

Co-authored-by: Andrew Rosiclair <arosiclair@expensify.com>

arosiclair

Thanks for the changes!

Gonals · 2023-03-06T15:45:16Z

Heyhey! Heads up that this PR caused this issue. We basically forgot to handle a case where the # was followed by two spaces and a new line

eVoloshchak · 2023-05-31T20:03:11Z

lib/ExpensiMark.js

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


👋
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

s77rt · 2023-07-15T18:13:39Z

lib/ExpensiMark.js

+            {
+                name: 'heading1',
+                regex: /^#(?!#) +(.+)$/gm,
+                replacement: '<h1>$1</h1>',
+            },


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)

cristipaval added 6 commits October 10, 2022 16:48

Add markdown to html rule.

bf8074e

Add html to markdown rule.

929abd2

Add tests for h1 to heading1

3ea9691

Remove generated markdown for h1 tags.

29fb855

Replace h1 with h2 in tests for generated markdown

56af2fe

Handle white spaces before and after h1 tags when html to markdown.

58f5320

cristipaval requested a review from a team as a code owner October 10, 2022 22:06

cristipaval self-assigned this Oct 10, 2022

melvin-bot bot requested review from arosiclair and removed request for a team October 10, 2022 22:07

Fix JS styling

650f1ac

arosiclair requested changes Oct 11, 2022

View reviewed changes

cristipaval and others added 3 commits October 12, 2022 14:28

require at least one character after # to avoid empty h1 tags

a1e6aa9

Co-authored-by: Andrew Rosiclair <arosiclair@expensify.com>

Split tests.

1f0a776

Split tests for html to h1 markdown conversion.

6cd14a4

cristipaval requested a review from arosiclair October 12, 2022 12:03

arosiclair approved these changes Oct 12, 2022

View reviewed changes

arosiclair merged commit 6010b3c into main Oct 12, 2022

arosiclair deleted the cristi_add-h1-markdown branch October 12, 2022 15:28

eVoloshchak mentioned this pull request Mar 6, 2023

[HOLD for payment 2023-03-01] [$1000] Editing header bold text in chat increases vertical gap above the text Expensify/App#14668

Closed

6 tasks

Gonals mentioned this pull request Mar 6, 2023

[HOLD for payment 2023-03-13] Empty line appears both in report and in LHN if # is followed by two or more spaces in multiline message Expensify/App#15372

Closed

6 tasks

eVoloshchak reviewed May 31, 2023

View reviewed changes

eVoloshchak mentioned this pull request May 31, 2023

[HOLD for payment 2023-05-25] [$1000] Line break before heading is missing in editing mode Expensify/App#17998

Closed

6 tasks

s77rt reviewed Jul 15, 2023

View reviewed changes

s77rt mentioned this pull request Jul 15, 2023

[HOLD for payment 2023-07-21] [$1000] Web - Heading after quote is not rendering Expensify/App#21846

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add h1 markdown #468

Add h1 markdown #468

cristipaval commented Oct 10, 2022

github-actions bot commented Oct 10, 2022 •

edited

Loading

cristipaval commented Oct 10, 2022

arosiclair Oct 11, 2022

cristipaval Oct 12, 2022

arosiclair left a comment

Gonals commented Mar 6, 2023

eVoloshchak May 31, 2023

s77rt Jul 15, 2023

Add h1 markdown #468

Add h1 markdown #468

Conversation

cristipaval commented Oct 10, 2022

Fixed Issues

Tests

QA

github-actions bot commented Oct 10, 2022 • edited Loading

cristipaval commented Oct 10, 2022

arosiclair Oct 11, 2022

Choose a reason for hiding this comment

cristipaval Oct 12, 2022

Choose a reason for hiding this comment

arosiclair left a comment

Choose a reason for hiding this comment

Gonals commented Mar 6, 2023

eVoloshchak May 31, 2023

Choose a reason for hiding this comment

s77rt Jul 15, 2023

Choose a reason for hiding this comment

github-actions bot commented Oct 10, 2022 •

edited

Loading