-
Notifications
You must be signed in to change notification settings - Fork 86
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
base: main
Are you sure you want to change the base?
Implement code and pre blocks support on web #456
Conversation
The E2E test is failing because |
src/web/utils/inputUtils.ts
Outdated
return BrowserUtils.isFirefox && node && (node.lastChild as HTMLElement)?.getAttribute('data-type') === 'codeblock' && node.lastChild?.lastChild?.lastChild?.lastChild?.nodeName === 'BR'; | ||
} | ||
|
||
// Parses the HTML structure of a MarkdownTextInputElement to a plain text string. Used for getting the correct value of the input element. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/web/utils/inputUtils.ts
Outdated
|
||
// On Firefox, when breaking one codeblock, its syntax and the <br> after it can be merged into the closing syntax of the previous codeblock. | ||
function didTwoCodeblocksMerged(node: ChildNode | null) { | ||
return BrowserUtils.isFirefox && node && (node.lastChild as HTMLElement)?.getAttribute('data-type') === 'codeblock' && node.lastChild?.lastChild?.lastChild?.lastChild?.nodeName === 'BR'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this not be a one liner ? It's a bit hard to read and node.lastChild?.lastChild?.lastChild?.lastChild?.nodeName
looks like magic :) I'd love to have it at least as a named variable for additional context
src/web/utils/inputUtils.ts
Outdated
|
||
// On Firefox, when breaking one codeblock, its syntax and the <br> after it can be merged into the closing syntax of the previous codeblock. | ||
function didTwoCodeblocksMerged(node: ChildNode | null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didTwoCodeblocksMerge
or areTwoCodeblocksMerged
@@ -81,11 +88,41 @@ function parseInnerHTMLToText(target: MarkdownTextInputElement, cursorPosition: | |||
} | |||
|
|||
if (node.nodeType === Node.TEXT_NODE) { | |||
let hasAddedNewline = false; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
addChildrenWithStyle(content, 'pre'); | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
const [_, lb, content] = node.children.join('').match(/^(\r?\n)([\s\S]*)$/) as RegExpMatchArray; | ||
ranges.push({type: 'codeblock', start: text.length, length: (content?.length ?? 0) + 7}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment explaining why + 7
or put it in named const ?
Details
Related Issues
GH_LINK
Manual Tests
Change codeFance rule in
/node_modules/expensify-common/dist/ExpensiMark.js
to code belowLinked PRs