Skip to content

Implement code and pre blocks support on web #456

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

Open
wants to merge 170 commits into
base: main
Choose a base branch
from

Conversation

BartoszGrajdek
Copy link
Collaborator

@BartoszGrajdek BartoszGrajdek commented Aug 21, 2024

Details

Related Issues

GH_LINK

Manual Tests

Change codeFance rule in /node_modules/expensify-common/dist/ExpensiMark.js to code below

             {
                name: 'codeFence',
                // ` is a backtick symbol we are matching on three of them before then after a new line character
                regex: /(?<![^^\r\n])(&#x60;&#x60;&#x60;(\r\n|\n))((?:\s*?(?!(?:\r\n|\n)?&#x60;&#x60;&#x60;(?!&#x60;))[\S])+\s*?(?:\r\n|\n))(&#x60;&#x60;&#x60;)/g,
                // We're using a function here to perform an additional replace on the content
                // inside the backticks because Android is not able to use <pre> tags and does
                // not respect whitespace characters at all like HTML does. We do not want to mess
                // with the new lines here since they need to be converted into <br>. And we don't
                // want to do this anywhere else since that would break HTML.
                // &nbsp; will create styling issues so use &#32;
                replacement: (_extras, _match, _g1, _g2, textWithinFences) => {
                    const group = textWithinFences.replace(/(?:(?![\n\r])\s)/g, '&#32;');
                    return `<pre>${group}</pre>`;
                },
                rawInputReplacement: (_extras, _match, _g1, newLineCharacter, textWithinFences) => {
                    const group = textWithinFences.replace(/(?:(?![\n\r])\s)/g, '&#32;').replace(/<emoji>|<\/emoji>/g, '');
                    return `<pre>${newLineCharacter}${group}</pre>`;
                },
            },

Linked PRs

@Skalakid
Copy link
Collaborator

Skalakid commented Jun 5, 2025

The E2E test is failing because expensify-common changes are missing in the GitHub actions. Will create PR with required changes and then patch the library in Live Markdown

@@ -81,11 +88,41 @@ function parseInnerHTMLToText(target: MarkdownTextInputElement, cursorPosition:
}

if (node.nodeType === Node.TEXT_NODE) {
let hasAddedNewline = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

the newline logic is confusing when adding codeblock after a codeblock or splitting one.
The input appears as in the new line, but it parses into the new codeblock only after manually pressing enter at the line break.

Screen.Recording.2025-06-16.at.18.08.56.mov

Copy link
Collaborator

Choose a reason for hiding this comment

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

One of the main goals of the Live Markdown Input is to preview how the message from Expensify will look after sending. In all scenarios, the code block syntax moves the content around it into separate lines (display: block). I agree that it can be a bit confusing, but we do the same thing for inline images

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe to make it more intuitive, we should require a whitespace after the codeblock closing syntax to be able to add another markdown after it. What do you think about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Feels different with demo image bc the image is adding w/o additional character between blocks. I think this is the expected behaviour here as well.

Screen.Recording.2025-06-18.at.10.57.05.mov

const currentLine = target.tree.childNodes[orderIndex]?.element;
const scrollTargetElement = currentLine || node.element;

if (!isChildOfMultilineMarkdownElement(node.element)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Found this multiline problem while testing, but it works same on main. IMO can be fixed as a followup.

Screen.Recording.2025-06-16.at.18.17.11.mov

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, will fix that in follow up PR :D

@Skalakid
Copy link
Collaborator

Skalakid commented Jul 4, 2025

Hello, here is the PR with required changes in the expensify-common library Expensify/expensify-common#865. Once it's merged we can proceed with with merging this PR

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