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.

Sign up for GitHub

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

Jump to bottom

feat: smart task marker in markdown #1080

Merged

seonim-ryu merged 10 commits into master from feat/markdown-smart-checkbox

Jul 14, 2020

Member

seonim-ryu commented Jul 7, 2020 •

edited

Loading

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

What is the smart task marker?

The smart task marker is a function that changes the marker syntax of the task according to user convenience in the Markdown editor.

Use case 1: entering key

Change the marker according to the value entered in the marker syntax.

When a value is entered in empty square brackets

Spaces are deleted only when the value entered in square brackets is x or X.

[ ] -> [ x] -> [x]
[ ] -> [x ] -> [x]

[ ] -> [y ] -> [y ]
[ ] -> [ y] -> [ y]
[ ] -> [y ] -> [yx ] -> [x ] -> [x]

[ ] -> [     ] -> [   x  ] -> [x]

When there is no value in the square brackets or it is deleted

Add one space in square brackets.

[x] -> [] -> [ ]
[y] -> [] -> [ ]

Use case 2: using shortcut

Change the marker when Shift + Ctrl + X is pressed in the task syntax. If the line is a task regardless of the cursor position, the state of the marker is toggled. If there are multiple lines of selection, the state of the markers should all change.

- [ ] foo| -> - [x] foo
- [ ] fo|o -> - [x] foo
- [ ]| foo -> - [x] foo
- [| ] foo -> - [x] foo
|  - [| ] foo -> - [x] foo

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

seonim-ryu added 6 commits

July 2, 2020 22:24


          wip: apply smart checkbox when entering key

1b23b86


          wip: change text to task state when using shortcut

c911a93


          wip: remove unused function and change shortcut

bd103a4


          wip: add unit test and change function name

6744b18


          refactor: change file name and terms

40a2cfe


          wip: change task marker when pasting

a9da581

seonim-ryu changed the title ~~feat: smart checkbox in markdown~~ feat: smart tasklist marker in markdown

seonim-ryu requested review from lja1018 and js87zz

July 8, 2020 03:11

js87zz reviewed

View reviewed changes

apps/editor/src/js/codemirror/tasklist.js Outdated

+                  for (let index = startLine, len = endLine; index <= len; index += 1) {
+                    mdNode = toastMark.findFirstNodeAtLine(index + 1);
+                    const { type } = mdNode;

Contributor

js87zz Jul 8, 2020 •

edited

Loading

이 부분은 두번만 쓰여서 할당하지 않고 바로쓰는게 더 깔끔하지 않을까요?

mdNode = toastMark.findFirstNodeAtLine(index + 1);

if (mdNode.type === 'list' || mdNode.type === 'item') {
  changeTaskState(mdNode, index, cm);
}

Member Author

seonim-ryu Jul 8, 2020

요건 항상 고민이네요. 프로퍼티에 여러 번 접근하고 싶지않아서 보통 변수에 할당해서 사용하는데
여기선 한줄로 줄이는게 더 깔끔해보이네요ㅎ

js87zz reviewed

View reviewed changes

apps/editor/src/js/codemirror/tasklist.js Outdated

+                return findClosestNode(
+                  mdNode,
+                  node =>
+                    (node.type === 'paragraph' || node.type === 'codeBlock') &&

Contributor

js87zz Jul 8, 2020

codeBlock도 체크할 필요가 있나요?

Member Author

seonim-ryu Jul 8, 2020

item 노드에 자식으로 올 수 있는 블록 요소가 파라그래프, 코드 블럭이 있어서 둘 다 체크했습니다~

Contributor

js87zz Jul 8, 2020 •

edited

Loading

음 테스트해보니 그런 경우 제대로 동작안하는 것 같은대요. task list 자식 노드로 코드블럭이 오는 경우를 제대로 고려하려면 지금보다 더 복잡해질 것 같은데.. 사실 거의 없는 사용성일 것 같은데 이런 케이스는 아예 제외하는게 어떨까요?

Member Author

seonim-ryu Jul 8, 2020

이렇게 동작하는 경우에 해당되는건데.. 지금 보니 에디터에서 하이라이팅이 이상하군요 -ㅁ-
마커 토글은 되지만.. 코드미러에서 태스크 > 코드블럭인 경우가 인덴트가 잘못 동작하네요. 저건 버그로 처리하겠습니다ㅎ

toastmark
editor

js87zz reviewed

View reviewed changes

apps/editor/src/js/codemirror/tasklist.js Outdated Show resolved Hide resolved

js87zz reviewed

View reviewed changes

apps/editor/src/js/codemirror/tasklist.js Outdated Show resolved Hide resolved

js87zz reviewed

View reviewed changes

apps/editor/src/js/codemirror/tasklist.js Outdated Show resolved Hide resolved

js87zz reviewed

View reviewed changes

apps/editor/src/js/codemirror/tasklist.js Outdated

+                    const taskMarkerLen = spaces + stateChar.length;
+                    const hasOnlySpaces = taskMarkerLen > 1 && !stateChar;
+                    if (!hasOnlySpaces) {

Contributor

js87zz Jul 8, 2020 •

edited

Loading

긍정조건으로 만들어 체크하는게 더 가독성에 좋을 것 같습니다~!

if (hasTaskMarker) {
  // ...
}

Member Author

seonim-ryu Jul 8, 2020

이 두 가지 케이스를 모두 커버하는 조건이라 변수명이라던지 조금 애매하네요. 요것도 다시 정리해보겠습니다~

[  x] -> [x]
[] -> [ ]

js87zz reviewed

View reviewed changes

apps/editor/src/js/codemirror/tasklist.js Outdated

+                        taskMarkerLen ? stateChar : ' ',
+                        startPos,
+                        addChPos(startPos, spaces ? spaces + 1 : 0)
+                      );

Contributor

js87zz Jul 8, 2020

아래처럼은 어떨까요?

const char = stateChar || ' ';

cm.replaceRange(char, startPos, addChPos(startPos, spaces ? spaces + 1 : 0));

js87zz reviewed

View reviewed changes

apps/editor/src/js/codemirror/tasklist.js Outdated Show resolved Hide resolved

js87zz reviewed

View reviewed changes

apps/editor/src/js/markdownEditor.js Outdated

+                  const { from, to, text } = ev;
+                  const changed = text.map(line => {
+                    const list = LIST_RX.exec(line);

Contributor

js87zz Jul 8, 2020

아.. 여긴 이렇게 정규식으로 체크할 수 밖에 없군요..

Contributor

js87zz Jul 8, 2020 •

edited

Loading

음 그런데 생각해보니까 붙여넣기 하고나서 change 이벤트가 발생하면 그 때 알아서 처리되지않나요? 현재는 changeTextToTaskMarker내의 로직이 커서 기준으로 처리되어서 마지막 줄만 처리되는데 change 이벤트에서 cmEvent 기준으로 처리하면 붙여넣기 할때 사전처리없이 알아서 변환될 것 같아요

Member Author

seonim-ryu Jul 8, 2020

그 방법을 시도를 안해본것은 아닙니다만.. 붙여넣기를 하는 경우에는 cm.replaceRange()가 라인 수만큼 발생해서 무한 루프에 빠지더라고요. 일단 정규식으로 선처리하는 방식이 아름답지 않은게 코드블럭 안에 리스트가 들어가는 경우를 체크할 수 없어서.. 요건 좀 더 확인해볼게요~

js87zz reviewed

View reviewed changes

apps/editor/test/unit/codemirror/tasklist.spec.js Outdated Show resolved Hide resolved

Contributor

js87zz commented Jul 8, 2020 •

edited

Loading

현재 task marker 안에서 스페이스를 입력하면 자동으로 변환해주는 로직 때문에 아무 동작을 안하는대요. 이런 경우는 버그처럼 보일 수도 있을 것 같아요. 공백이나 탭키가 입력되었을 때는 변환되지 않도록 막는게 맞지 않을까요? 편의를 위해서 자동으로 변환해주는 것은 좋지만 사용자의 키 입력 동작을 막아버리면 안될 것 같아요.

js87zz approved these changes

View reviewed changes

Contributor

js87zz left a comment

리뷰 완료입니다. 단축키가 생기니까 훨씬 편하네요~! :)

Contributor

lja1018 commented Jul 8, 2020

[07/08] 리뷰완료.
고생하셨습니다!


          feat: apply code review

6ba797e

seonim-ryu force-pushed the feat/markdown-smart-checkbox branch 2 times, most recently from 2e01a7b to 69d92e7 Compare

July 14, 2020 02:17


          fix: remove logic to set value

6fecca5

seonim-ryu force-pushed the feat/markdown-smart-checkbox branch from 7057fba to 6fecca5 Compare

July 14, 2020 02:31


          chore: edit unit test of spec

0942b45

js87zz approved these changes

View reviewed changes

Contributor

js87zz left a comment

깔끔하게 끝났네요. 고생하셨습니다! 👍

lja1018 approved these changes

View reviewed changes

Contributor

lja1018 left a comment

[07/14] 리뷰완료.
고생하셨습니다~!

apps/editor/test/unit/markdownEditor.spec.js Outdated Show resolved Hide resolved


          feat: apply code review

a45edbf

seonim-ryu changed the title ~~feat: smart tasklist marker in markdown~~ feat: smart task marker in markdown

seonim-ryu added Feature Mode: Markdown labels

seonim-ryu added this to the 2.3.0 milestone

seonim-ryu merged commit d0b7501 into master

seonim-ryu deleted the feat/markdown-smart-checkbox branch

July 14, 2020 14:11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Mode: Markdown