Refactor: Move typecasting outside of "insertMarkdownText"
The following discussion from !44332 (merged) should be addressed:
-
@justin_ho started a discussion: (+3 comments) Question (non-blocking): I might not have the full context but just wanted to check if we have considered normalizing the input before it is sent to this function? Asking because
insertMarkdownText
is already quite complex as it is and adding extra responsibility of normalizing its own params doesn't really help.Here is something I tested that works the same:
diff --git a/app/assets/javascripts/lib/utils/text_markdown.js b/app/assets/javascripts/lib/utils/text_markdown.js index f4c6e4e3584..64ae9866c25 100644 --- a/app/assets/javascripts/lib/utils/text_markdown.js +++ b/app/assets/javascripts/lib/utils/text_markdown.js @@ -290,7 +290,7 @@ function updateText({ textArea, tag, cursorOffset, blockTag, wrap, select, tagCo const $textArea = $(textArea); textArea = $textArea.get(0); const text = $textArea.val(); - const selected = selectedText(text, textArea) || tagContent; + const selected = (selectedText(text, textArea) || tagContent).toString(); $textArea.focus(); return insertMarkdownText({ textArea, @@ -378,7 +378,7 @@ export function addEditorMarkdownListeners(editor) { blockTag: mdBlock, wrap: !mdPrepend, select: mdSelect, - selected: editor.getSelectedText(), + selected: editor.getSelectedText().toString(), text: editor.getValue(), editor, });
Not saying the above approach is better per-se but just wanted to raise the idea.
I agree with @justin_ho that any type casting required for
insertMarkdownText
to work properly should happen outside