feat: enhance syntax highlighting of markdown editor #910

seonim-ryu · 2020-04-20T01:22:29Z

Please check if the PR fulfills these requirements

It's the right issue type on the title
When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
The commit message follows our guidelines
Tests for the changes have been added (for bug fixes/features)
Docs have been added/updated (for bug fixes/features)
It does not introduce a breaking change or has a description of the breaking change

Description

The syntax highlighting function of the editing area (left side) in the markdown editor has been enhanced. Previously, the default theme style of CodeMirror was used in the Editor, but it is changed to fit the TOAST UI's design.

Bundle File Changes

The Editor and Viewer styles have been replaced with improved designs, and ${prefix}-old.css style files are provided for users who want the previous style.

v2.0

- dist/
    - toastui-editor.css
    - toastui-editor-viewer.css
    - cdn/
        - toastui-editor.css
        - toastui-editor.min.css
        - toastui-editor-viewer.css
        - toastui-editor-viewer.min.css

v2.1

- dist/
    - toastui-editor.css
    - toastui-editor-viewer.css
    - toastui-layout.css
    - toastui-editor-old.css // added
    - toastui-editor-viewer-old.css // added
    - cdn/
        - toastui-editor.css
        - toastui-editor.min.css
        - toastui-editor-viewer.css
        - toastui-editor-viewer.min.css
        - toastui-layout.css
        - toastui-layout.min.css
        - toastui-editor-old.css // added
        - toastui-editor-old.min.css // added
        - toastui-editor-viewer-old.css // added
        - toastui-edtiro-viewer-old.min.css // added

Other Changes

The old folder was added to the source folder to separate it from the previous style.

v2.0

- src/css/
    - toastui-editor.css
    - toastui-editor-contents.css

v2.1

- src/css/
    - editor.css
    - contents.css  
    - md-syntax-highlighting.css
    - old/
        - contents.css  
        - md-syntax-highlighting.css

Demo

Download test.zip

Use `old.css`

zip file > editor-old-style.html

Use New Style

zip file > editor-new-style.html

Thank you for your contribution to TOAST UI product. 🎉 😘 ✨

seonim-ryu · 2020-04-20T01:43:49Z

apps/editor/src/css/old/md-syntax-highlighting.css

@@ -0,0 +1,78 @@
+.te-md-container .CodeMirror {


TODO: old 폴더의 스타일은 디자인 확인 이후 수정

seonim-ryu · 2020-04-20T01:44:16Z

apps/editor/src/css/old/contents.css

@@ -0,0 +1,256 @@
+@charset "utf-8";


TODO: old 폴더의 스타일은 디자인 확인 이후 수정

apps/editor/src/js/markdownEditor.js

js87zz · 2020-04-20T06:48:15Z

apps/editor/src/js/indexOldStyle.js

+import '../css/editor.css';
+import '../css/contents.css';
+import '../css/old/md-syntax-highlighting.css';


기존 css 번들을 위해 추가된 파일인건가요?

넵. 이 엔트리 파일은 toastui-editor-old.css 파일을 생성하기 위해서 추가되었습니다~

apps/editor/src/js/markdownEditor.js

js87zz · 2020-04-20T07:41:03Z

apps/editor/src/js/markdownEditor.js

      );
    }

-    this._markTextInListItemChildren(node.firstChild, className);
+    this._markTextInListItemChildren(node.firstChild, `${className} tui-md-marked-text`);


@seonim-ryu @dongwoo-kim
개인적으로 mark 관련된 메서드들이 특별히 컨텍스트에 얽히는게 없어보이고 꽤나 많아서 따로 class로 분리해서 관리하는 것도 가독성이나 관리 측면에서 좋을 것 같다는 생각이 드는대요. markDownEditor 에서는 동일한 인터페이스의 API를 사용해서 더 깔끔하게 정리될 것 같기도 하구요. 어떠신가요?

저는 클래스 대신 헬퍼로 분리하는걸 생각했었는데요. 그러면 코드미러 객체(this.cm)를 처리하기가 애매해지더라고요. 매번 파라미터로 넘기자니 기존 에디터 구조와 맞지 않는 것 같고요.

import { markTextInListItemChildren } from './helper/markMarkdownText'; // ... markTextInListItemChildren();

그러면 말씀하신대로 클래스로 생성해서 사용하면, 이런 구조가 되려나요??

class MarkdownEditor extends CodeMirrorExt { constructor( /* */ ) { // ... this.markTextManager = new MarkTextManager(this.cm); } _markNodes( /* */ ) { // ... this.markTextManager.markTextInListItemChildren(); } }

넵 아니면 type 까지 넘겨서 markTextManager 내부에서 모두 처리하거나 그렇지 않아도 인터페이스 자체는 통일하여 에디터 쪽에서는 조건문 없이 깔끔하게 사용할 수도 있을 것 같습니다~!

@seonim-ryu
매니저는.. 그만 만들었으면 좋겠습니다. 기존 것도 없애고 싶은데.. ㅋ
지금 로직만 봐서는 cm.getLine() 데이터를 넘겨주고, mark와 lineClass 정보만 넘겨받으면 굳이 this.cm을 관리하지 않아도 될 것 같습니다. 저라면 그냥 모듈을 하나 만들고, 함수 하나만 export 해서 처리할 것 같네요.

@seonim-ryu
동우 책임님이 말씀하신 것처럼 꼭 class 가 아니더라도 별도의 모듈로 분리해서 인터페이스만 잘 정리되면 좋을 것 같습니다. 에디터 전반적으로 OOP 지향적으로 되어 있어서 class를 예로 들었는데, 단순한 모듈로 깔끔하게 정의할 수 있으면 그것도 좋습니다~!

markTextHelper.js 파일 추가로 변경
(검토 사항 : 파일명 markTextHelper or markNodeHelper)

f6b46ba

apps/editor/src/js/markdownEditor.js

js87zz · 2020-04-20T07:59:06Z

apps/editor/src/js/markdownEditor.js

+    }
+
+    this._markText(openDelimEnd, closeDelimStart, 'tui-md-marked-text');
+    this._markCodeBlockLine(startLine, endLine);
  }

  _markTextInListItemChildren(node, className) {


blockquote에서도 사용되고 있어서 네이밍을 변경하면 좋을 것 같은데..마땅한게 떠오르지 않네요..

~~_markBlockQuote 메서드에서 두 번째 조건문이 인용구 안에 리스트가 들어간 경우를 처리한거라서 네이밍은 유지해도 될 것 같습니다~~

tui.editor/apps/editor/src/js/markdownEditor.js

Lines 431 to 439 in f2fca2d

_markBlockQuote(node, start, end) {

if (node.parent && node.parent.type !== 'blockQuote') {

this._markText(start, end, `tui-md-block-quote`);

}

if (node.firstChild) {

this._markTextInListItemChildren(node.firstChild, 'tui-md-marked-text');

}

}

로직을 보면 인용문안에 리스트가 없어도 문장이나 코드블럭이 있는 경우면 다 실행될 것 같은대요. 중첩된 노드를 가질 수 있는 노드안에 텍스트 처리를 한다는 의미가 드러나면 더 좋지 않을까요?

아래 버그를 수정하다보니 로직이 잘못된 부분이 있네요. 관련해서 메서드명 변경할 수 있게 되면 코멘트 남겨둘게요~

js87zz · 2020-04-20T08:10:02Z

apps/editor/src/js/markdownEditor.js

+  _getClassNameOfListItem(node) {
+    let depth = 0;
+
+    while (node.parent.parent && node.parent.parent.type === 'item') {


while 문을 돌다보면 node.parent 가 null 인 경우도 있을 것 같은데 이런 경우는 없었나요?

이 메서드에 전달되는 node는 리스트 아이템(<item>)이고 항상 부모(<list>)가 존재하기 때문에 null인 경우는 없을 것 같습니다ㅎ

js87zz · 2020-04-20T09:16:41Z

@seonim-ryu
아래처럼 간혹 하이라이팅이 잘못되는 경우가 있어 올려드립니다. 확인 부탁드려요!

인용문 기호 하이라이팅이 풀리는 현상
중첩된 요소에서 코드 블럭을 지울 경우 마크가 남는 현상

seonim-ryu · 2020-04-20T09:36:29Z

@seonim-ryu
아래처럼 간혹 하이라이팅이 잘못되는 경우가 있어 올려드립니다. 확인 부탁드려요!

인용문 기호 하이라이팅이 풀리는 현상

중첩된 요소에서 코드 블럭을 지울 경우 마크가 남는 현상

1번은 해결되었던건데 리팩토링하면서 발생한 이슈로 보이고, 2번은 텍스트가 비어있을 때 이벤트가 발생하지 않아서 생긴 문제 같네요. 확인해보겠습니다~

js87zz

리뷰 완료입니다. 하이라이팅 처리되니 확실히 훨씬 보기 좋네요! 고생하셨습니다 :)

seonim-ryu · 2020-04-20T15:40:43Z

apps/editor/src/js/markdownEditor.js

@@ -18,6 +18,7 @@ import {
  getMdStartCh,
  getMdEndCh
 } from './utils/markdown';
+import { getMarkInfo } from './markTextHelper';


TODO: helper 폴더 추가 및 이동 (검토 필요)

나중에 폴더 구조 잡고 옮기면 파일명에 Helper suffix도 제거하면 좋을 것 같습니다~!

apps/editor/src/js/markTextHelper.js

seonim-ryu · 2020-04-21T09:56:39Z

apps/editor/src/js/markdownEditor.js

@@ -258,6 +256,8 @@ class MarkdownEditor extends CodeMirrorExt {
        }
      }

+      this._removeBackgroundOfLines();


TODO : ToastMark 수정 필요 (특정 상황에서 코드블럭의 백그라운드가 제거되지 않음)

apps/editor/src/js/markTextHelper.js

dongwoo-kim · 2020-04-21T10:55:06Z

apps/editor/src/js/markTextHelper.js

+  const { level } = node;
+
+  marks.push(createMarkInfo(start, end, cls(['heading', `heading${level}`])));
+  marks.push(createMarkInfo(start, { line: start.line, ch: start.ch + level }, classNameMap.DELIM));


cls를 사용할 때와 classNameMap을 사용할 때의 차이를 잘 모르겠습니다.
모든 클래스를 맵으로 관리할 게 아니라면 classNameMap이 없는 편이 더 낫지 않을까요?

classNameMap은 2번 이상 사용되는 클래스들을 모은 것이고 cls는 각 함수 안에서 한번씩 사용하는 것들로 분리했습니다.

apps/editor/src/js/markTextHelper.js

seonim-ryu requested review from dongwoo-kim and js87zz April 20, 2020 01:22

seonim-ryu commented Apr 20, 2020

View reviewed changes

seonim-ryu force-pushed the feat/advance-markdown-syntax-highlighting branch 2 times, most recently from e92faea to d2ef5b1 Compare April 20, 2020 02:32

seonim-ryu changed the title ~~feat: advance syntax highlighting of markdown editor~~ feat: enhance syntax highlighting of markdown editor Apr 20, 2020

seonim-ryu added 3 commits April 20, 2020 11:40

wip: advance syntax highlighting of markdown editor

8615ea7

wip: add background of fenced code block

610c1d6

chore: change path of edtior style in karma

f2fca2d

seonim-ryu force-pushed the feat/advance-markdown-syntax-highlighting branch from d2ef5b1 to f2fca2d Compare April 20, 2020 02:47

seonim-ryu added this to the 2.1.0 milestone Apr 20, 2020

dongwoo-kim reviewed Apr 20, 2020

View reviewed changes

apps/editor/src/js/markdownEditor.js Outdated Show resolved Hide resolved

js87zz reviewed Apr 20, 2020

View reviewed changes

dongwoo-kim reviewed Apr 20, 2020

View reviewed changes

apps/editor/src/js/markdownEditor.js Outdated Show resolved Hide resolved

dongwoo-kim reviewed Apr 20, 2020

View reviewed changes

apps/editor/src/js/markdownEditor.js Outdated Show resolved Hide resolved

dongwoo-kim reviewed Apr 20, 2020

View reviewed changes

apps/editor/src/js/markdownEditor.js Outdated Show resolved Hide resolved

js87zz reviewed Apr 20, 2020

View reviewed changes

apps/editor/src/js/markdownEditor.js Outdated Show resolved Hide resolved

js87zz reviewed Apr 20, 2020

View reviewed changes

apps/editor/src/js/markdownEditor.js Outdated Show resolved Hide resolved

js87zz reviewed Apr 20, 2020

View reviewed changes

apps/editor/src/js/markdownEditor.js Outdated Show resolved Hide resolved

js87zz reviewed Apr 20, 2020

View reviewed changes

js87zz approved these changes Apr 20, 2020

View reviewed changes

seonim-ryu added 2 commits April 20, 2020 20:44

wip: apply code review

1e8e3f2

wip: change structure of marking node in markdown editor

f6b46ba

seonim-ryu commented Apr 20, 2020

View reviewed changes

wip: change logic to use class name

d0d336c

wip: change logic of getMarkInfo function

33c9cd4

seonim-ryu commented Apr 21, 2020

View reviewed changes

apps/editor/src/js/markTextHelper.js Outdated Show resolved Hide resolved

fix: broken syntax highlighting of block quote and code block

2e2e306

seonim-ryu force-pushed the feat/advance-markdown-syntax-highlighting branch from d9c79ab to 2e2e306 Compare April 21, 2020 09:36

seonim-ryu commented Apr 21, 2020

View reviewed changes